-
Notifications
You must be signed in to change notification settings - Fork 214
fix(nimbus): make boolean text more visible in dark mode in codemirror windows #13596
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
base: main
Are you sure you want to change the base?
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.
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
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.
|
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 |
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.
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 🙏
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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






Because
This commit
Fixes #13594