Option to use Firefox theme for Dark Reader GUI #13309
Conversation
|
I won't touch it as it changes too many things in the code but...
And after looking into the feature request. I (and I bet Anton) thought we want to get colors from Firefox theme and apply them into Dark Reader color schemes: But here you only change the colors of Dark Reader popup. Is it really worth that changes in the code? P.S. Assigning @alexanderby to review it. |
|
There should be no option, you can see that here, it only appears on firefox: https://github.com/darkreader/darkreader/blob/3f4cfccaf502636471d9f07b589aa16fb9705ca9/src/ui/options/general/general-tab.tsx Not sure what you wanted back then, i opened the feature request two years ago for what I implemented. But, there could be another separate one, for checking if the theme is available, and putting it in that list, after some conversion. And yes, it's worth it, like themed apps are worth it. |
Just when I went through that Feature Request my thoughts what was requested was totally different that's why I mentioned my thoughts. Thanks for the rest of the screens and more info how it will work. Still I am only cleaner here to check and make CSS fixes and other not much complicated things in Config. |
There was a problem hiding this comment.
Dear @MorsMortium,
Thank you for your interest in Dark Reader. Please understand Dark Reader is used by millions. What solves your problem can make the app harder to use for others and can make the code harder to maintain.
I cannot remember any other request to change the colors of our user interface. The result doesn't look visually appealing to me. The app's design will change completely soon.
However I appreciate your effort and reviewed your Pull Request. I hope it will help you learn more about JavaScript best practices, naming conventions and available browser APIs.
Regards,
Alexander.
src/ui/utils.ts
Outdated
| var Style = Parser.parseFromString(` | ||
| <style> | ||
| * { | ||
| color: ${Color} !important; |
There was a problem hiding this comment.
* and !important are discouraged for theming.
src/ui/utils.ts
Outdated
| background-image: unset; | ||
| } | ||
| html, button, .tab-panel__tab, .toggle__btn, .updown__icon, .track--clickable, .toggle::before, .header__more-settings, .multi-switch, .checkbox, .header__more-settings__shortcut, .news__list, .select__option, .select__list, .news__header, .check-button .checkbox__checkmark, .dropdown__list__item, .message-box, .dropdown__list, .editor, .preview .page, .preview .news-section, .preview .news-section__popover, .color-picker__hsb-line, .site-list__textbox, .time-range-picker__input, .automation__location-control__latitude, .automation__location-control__longitude, .text-list__textbox, .select__textbox, .dynamic-per-site__search-input { |
There was a problem hiding this comment.
Listing selectors within the app is absolutely not scaleable and unmaintainable. CSS variables (custom properties) should be used and redeclared like :root { --color-bg: #000; ... }.
There was a problem hiding this comment.
Do you mean the entire css of the entire extension should be rewritten for this?
There was a problem hiding this comment.
We have theme.less file with LESS variables. In theory you should be able to have something like:
:root {
--color-back: #141e24;
}
@color-back: var(--color-back);And then you can insert CSS like
:root {
--color-back: #000000;
}and the colors within the entire UI should change. However there could be issues with the colors of icons (they should be moved from LESS to HTML/JSX markup (currentcolor can be used for SVG to make it use CSS color value). Another issue could be LESS color mixing functions (like fade or mix), they won't process the CSS variables.
Please understand that even if you do all those changes, the PR most likely won't be merged due to reasons I described earlier. However it could be useful to replace LESS color variables with CSS variables in our code. If you would like to have some practice, you can submit a separate PR addressing these color variables. Then another for moving icons from CSS to SVG (we have src/ui/icons folder, see how they are used in the code).
There was a problem hiding this comment.
Please understand that even if you do all those changes, the PR most likely won't be merged due to reasons I described earlier.
Do you mean that you don't like the look of it? I'm also not sure how it would make it harder to use for others, while it's an off by default setting, that doesn't even appear to most users (chromium)
Sorry, but I made the PR to implement a feature, not to refactor code, so if the feature won't be merged, then it's to no use to me. (if it would be merged, i would do all changes, which i did start already)
There was a problem hiding this comment.
@alexanderby I did clean it up as much as I could (while still using css selectors/classes). I'd like an answer to my question. If it wouldn't be merged even with using css variables, I'll just close the pr/issue and keep it for my own usage
Property name change
Css order
Missing whitespaces
Better option label/description
Don't check settings if not needed
Function name change
Use utils/color functions
Const and let instead of var
Single quote strings
Template strings
Use document.createElement('style')
| } | ||
| /* Icon inversions */ | ||
| .settings-button-icon, .m-help-button__text::before, .nav-button::after, |
There was a problem hiding this comment.
Sorry, but I cannot merge this implementation (as I said earlier it's very hard to maintain). I have some ideas about changing the UI theme dynamically, and when I get to refactoring the use of color variables it will be easier for you to implement your option.
3 participants
Fixes #10441
Disabled: