-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
color value was in quotes, was bad syntax. tested by making it #3f3 b… #1094
Conversation
…right lime green temporarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rleir -- the removal of quotation marks is good, but the hex value should either be #333
or #333333
(both are valid CSS as far as I'm aware), not #010101
as introduced in this PR
Hi James, @jameshadfield The original design intent seems to be to control text color centrally for the whole app. If we had time on our hands we could remove other text color settings throughout the app, and control all text color from this one line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit more complex than first suspected I believe. You are absolutely correct that "#333"
is invalid and thus ignored. However the fallback color differs across the app, depending on the element in question's hierarchy. For instance, the footer text falls back to "rgb(136, 136, 136)" (grey) not #010101. I think the best solution is just to remove color
entirely from the global CSS here which matches the current app behavior.
(The Integration tests most probably don't cover the footer text yet) |
Agreed, let's remove it. I will make the change in the PR when I get a moment. |
Hi James @jameshadfield To pass the integration test, your options are color: #010101 or remove this attribute completely here as has been done. Now moving on to more important issues (suggestions please?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rleir for your work here and elsewhere. I'll update the list of open issues etc (https://github.com/orgs/nextstrain/projects/5) tonight
…right lime green temporarily.
Description of proposed changes
What is the goal of this pull request? What does this pull request change?
Fix invalid CSS detected by a linter. My goal is mainly to clean all the minor lint warnings so the linter becomes useful again.
Related issue(s)
Fixes #1081
Related to #
Testing
What steps should be taken to test the changes you've proposed?
I tested by setting the color value to 3f3 bright lime green temporarily. It only affects the entropy scale lines and the text 'This work is made possible by the open...'. It would probably have historically colored most of the text in the UI. When it was not working someone corrected the color by creating 'darkgrey' or similar.
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?
No change to the tests but I can do if you would like me to.
Thank you for contributing to Nextstrain!