-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Feat: Add theme chooser #2180
Feat: Add theme chooser #2180
Conversation
…-react into feat/theme-overhaul
@jeremyphilemon @shuklaayush @JunaidBabu @sudevschiz Sorry maintainers, I had to tag you. Due to the gravity of PR, a large number of changes, you may want to review this first where it will create a conflict (where dark-theme related variables are present). Also, I see a ton of PR related to style changes. To minimize conflicts and resolution time, can you please look into this first? |
this is very good i would love this pr merged @balrajsingh9 👍 |
Thanks. I'm awaiting review and can't wait for everyone checking this out! 😄 |
@balrajsingh9 can #2158 be done along with this? |
Absolutely yes. We can make the scrollbars (only webkit based browsers) to use the same color scheme(primary color) being used by the application. I'll try it out and will post the screenshots. Then we can decide if it is ok or looks too fancy or funky 😅 |
Updated the styles that came in the last merge. @shuklaayush @jeremyphilemon I think you guys might be super busy. If you don't mind can you please add more maintainers to review the PR(s)? Just a suggestion. 😄 |
@balrajsingh9 Hey, we'd love to add more maintainers, but not too many people are familiar with the codebase. We're trying to maintain good code quality while minimizing the actual code that gets pushed into the repo. This way it's easier for us to maintain. There have been situations in the past where some new feature was merged, but then later the developer moved on with his/her life and we had to end up cleaning/updating their code to make sure it's still usable (and doesn't crash the website). A lot of people depend on the website for updates, so I hope you can understand why we're being really careful with what gets merged :). |
…-react into feat/theme-overhaul
…-react into feat/theme-overhaul
2f8a4f7
to
2a42f3b
Compare
@shuklaayush Can we do something about this PR? as a small change in |
…-react into feat/theme-overhaul
Hey @balrajsingh9, I checked it out just now. There are a few small things here and there in dark mode which look a bit weird. But that's beside the point. My primary concern is that, from a normal user standpoint, this doesn't really simplify things. Switching from light to dark mode is now two clicks away instead of one. From a maintenance point of view, we'll have to add more SVG filters in order to make the choropleth map look natural on other background colours. So there's a bit more complexity added to the code as well. I'm not sure how much extra effort will have to be put in to maintain these different colour themes. Anyways, that's my opinion. Maybe @jeremyphilemon will have different thoughts |
@shuklaayush The issue in this context (and several others were created similarly) is regarding the difficulty in reading texts and some other parts of the application when switched to current(deployed) dark mode. This is the primary reason why I made these changes, the customization is just cherry on top. I fathom your concern regarding the two clicks. There are always other ways to refactor this to maintain customizability. I have some ideas to overcome the extra work and I have stated in some other issue as well that the user will hardly change the colors once settled on some customization, but again your concern is valid. I would like to improve the UX flow given that @jeremyphilemon gives a green signal to this change and doesn't close this due to some other reason(s). |
@shuklaayush Please feel free to close this PR, if you think the changes are not required. Whenever any change is done inside Let me know if you think there is even a small chance of getting this merged. I'm just sad. I worked hard on this... 😞 |
Hi @balrajsingh9, sorry that we couldn't actively work on this PR. We indeed discussed about this, but due to other priorities, we couldn't focus on the theme picker right now. Really sorry, but as you said, I will close this PR. We are making some major changes internally and post that, we will be updating the roadmap with new tasks that need help. Hope that you will join in on the discussion then and contribute. Once again, sorry that your hard work did not get merged. |
Hi @sudevschiz, it is fine 👍 And really glad to know that new features/tasks are coming. I'd be happy to contribute. Please let me know, if possible, when you have the new roadmap ready. |
Description of PR
Dark theme completely overhauled. Written from scratch and all buggy colors are fixed.
Introduces a new way to choose the theme and primary color(thanks to
twitter
) 👍Default
,Dim
,Lights Out
localStorage
to cache the color-combination.Relevant Issues
Fixes #1417
Checklist
Screenshots