Skip to content
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

Use CSS variables in getColor #286

Merged
merged 3 commits into from
Mar 24, 2021
Merged

Use CSS variables in getColor #286

merged 3 commits into from
Mar 24, 2021

Conversation

jadebuckwalter
Copy link
Collaborator

Instead of hard-coding colors, use CSS variables so that grades are
more visible in dark mode.

Instead of hard-coding colors, use CSS variables so that grades are
more visible in dark mode.
@jadebuckwalter
Copy link
Collaborator Author

This fixes the original issue. But in the process I came across lightColors - what is that used for, and should those colors get CSS variables as well?

@jadebuckwalter jadebuckwalter linked an issue Mar 24, 2021 that may be closed by this pull request
@psvenk psvenk added this to the 2.7.1 milestone Mar 24, 2021
@psvenk psvenk self-requested a review March 24, 2021 22:02
@psvenk
Copy link
Member

psvenk commented Mar 24, 2021

This fixes the original issue. But in the process I came across lightColors - what is that used for, and should those colors get CSS variables as well?

Apparently this was added in a43af60 but it seems that it was not used at all then and is not used now, so I think it can be safely deleted (along with getLightColor).

Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good, but I think it would be good to have a separate color (dark orange?) for D-range grades, both for maintaining consistency (every other letter grade has its own color) and because we shouldn't use red for a D when it is still a passing grade.

Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ready to squash and merge.

Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I (semi-)retract my approval. I think it may be a good idea to compute the color first (using the variable), given that the color (as returned by getColor) will be entered into the JSON export and therefore should be portable.

This would involve re-computing all colors upon switching themes, which might be a little much. @jadebuckwalter Your thoughts?

@psvenk
Copy link
Member

psvenk commented Mar 24, 2021

Or, we could keep using CSS variables within currentTableData and just make sure that we do not remove any variables in the future to maintain backwards compatibility. This breaks forwards compatibility, but we never guaranteed forwards compatibility in the first place. It would also be good to change green to var(--green) inside newAssignment in buttonFunctions.js.

Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the repeated comments. After 3f2819c, I think the status quo with the backwards-compatibility guarantee is the way to go, given that it's the least disruptive solution. @jadebuckwalter, feel free to squash-and-merge if you agree.

@jadebuckwalter jadebuckwalter merged commit 3d87a64 into master Mar 24, 2021
@jadebuckwalter jadebuckwalter deleted the get-color branch March 24, 2021 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use CSS variables in getColor
2 participants