fix(nimbus): make boolean text more visible in dark mode in codemirror windows#13596
fix(nimbus): make boolean text more visible in dark mode in codemirror windows#13596moibra05 merged 6 commits intomozilla:mainfrom
Conversation
moibra05
requested review from
freshstrangemusic,
jaredlockhart and
yashikakhurana
as code owners
|
yashikakhurana
left a comment
yashikakhurana
left a comment
There was a problem hiding this comment.
Thank you @moibra05 this is much readable now, we have codemirror on other pages too we want to make sure we add at all the places like summary page
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Can't we just use some of the out of the box themes that specifically are for light/dark modes like
?
I don't believe there are any default themes included in the core cm6 library but it is possible to install and integrate third party ones |
|
Yeah I'm just curious if that's a cleaner solution than managing the colours explicitly? @moibra05 @yashikakhurana what do you think? |
|
Incorporating themes would be a more robust solution but based on some research i did, it looks like having it dynamically switch between dark/light modes would add a bit of complexity |
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Okay well better to go with the simple solution first to get something out the door and then we if we want/need to we can spend more time on a broader solution later. Thnx for hopping on this quick 🙏
moibra05
force-pushed
the
13594
branch
2 times, most recently
from
8a2a276 to
a76632f
Compare
| basicSetup, | ||
| lineNumbers(), | ||
| drawSelection(), | ||
| highlightSpecialChars(), | ||
| foldGutter(), | ||
| autocompletion(), | ||
| bracketMatching(), | ||
| closeBrackets(), | ||
| highlightSelectionMatches(), | ||
| indentOnInput(), | ||
| highlightActiveLine(), | ||
| syntaxHighlighting(highlightStyle), | ||
| syntaxHighlighting(defaultHighlightStyle), | ||
| history(), | ||
| search(), | ||
| keymap.of([...defaultKeymap, ...searchKeymap, ...historyKeymap]), |
There was a problem hiding this comment.
Yeah this is a bit hairy. If using dedicated light/dark themes is too complicated let's just go back to your original approach of just overriding the colour of the bools. Simplest, smallest solution is usually best 🙏
There was a problem hiding this comment.
The approach i have here is only changing the colour of the boolean text. basicSetup packages all of these extensions without any option to exclude specific ones, which is an issue because this included extension prevents me from setting a custom HighlightStyle that overrides boolean text colour. Annoyingly, the only way to fix this is to manually include all the extensions except defaultHighlightStyle
moibra05
dismissed stale reviews from yashikakhurana and jaredlockhart
addressed changes
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
4 participants
Because
This commit
Fixes #13594