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
Support colorblind variation of default theme #9799
Support colorblind variation of default theme #9799
Conversation
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.
🚀
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.
Great
A few comments only
|
||
_context.Controller.ShowThemeSettings(); | ||
|
||
_context.Page.IsChoosingThemeVariationsEnabled.Should().BeTrue(); |
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.
IsChoosingThemeVariationsEnabled is always enabled, property can be removed
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.
done
.Branch { color: #00d76b; } | ||
.Branch.colorblind { color: #0080ff; } /* hsl(210, 100%, 50%) */ | ||
|
||
.RemoteBranch { color: #ff8080; } /* hsl(0, 100%, 75%) */ |
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.
nit pick and not from this PR: Can the colors be ordered as in the other themes so they are easier to compare?
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.
done
{ | ||
"colorblind", new() | ||
{ | ||
{ AppColor.Graph, Color.FromArgb(0x06, 0x00, 0xa8) }, |
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.
Question: The invarientTheme is read from invariant.css, these are hardcoded in the app.
Is there a reason for this?
I would expect the invariant theme to be hardcoded, but it is nice to have the colors listed in a file too.
(Still hoping themes can return...)
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.
I agree, I don't like having both hardcoded colors and .css colors either. Would rather keep everything in .css for simplicity
Proposed changes
Default theme uses hardoded color values. Add hardcoded color values for colorblind variation as well.
Screenshots
Test methodology
Tested manually
ColorSettingsPage logic to not disable colorblind checkbox is covered by unit test
Test environment(s)
Windows 10
Merge strategy
✒️ I contribute this code under The Developer Certificate of Origin.