Dark mode: allow user to set it #296
Conversation
jasonpaulso
commented
Apr 22, 2024
jasonpaulso
commented
- [app/controllers/settings/people_controller.rb]: adding preference key to person_params
- [app/controllers/users_controller.rb]: adding preference key to user_params
- [app/javascript/controllers/dark_mode_controller.js]: adding DarkModeController
- [app/views/application/_head.html.erb]: it seems necessary to add some scripting to head to initially set the dark mode class in order to prevent FOUC on page load/refresh. Additionally, adding logic to handle un-signed in user dark mode states.
- [app/views/layouts/application.html.erb]: layout setup for DarkModeController
- [app/views/settings/people/_form.html.erb]: adding dork mode toggles
- [config/tailwind.config.js]: dark mode configuration
…ML-encoded entities to display and add tests for verification.
…ML-encoded entities to display and add tests for verification.
… block_code * fix(messages_helper.rb): remove unnecessary empty line * fix(custom_html_renderer.rb): remove unused initialize method * fix(custom_html_renderer.rb): remove unnecessary empty line
… block_code * fix(messages_helper.rb): remove unnecessary empty line * fix(custom_html_renderer.rb): remove unused initialize method * fix(custom_html_renderer.rb): remove unnecessary empty line
… block_code * fix(messages_helper.rb): remove unnecessary empty line * fix(custom_html_renderer.rb): remove unused initialize method * fix(custom_html_renderer.rb): remove unnecessary empty line
…puts for theme setting * feat(application/_head.html.erb): add theme partial * feat(application/head/_theme.html.erb): add theme script based on user preferences or system preference Closes Allow user to toggle dark/light mode via settings AllYourBot#169
|
@krschacht Here is a cleaner PR. I've made most/all? of the changes you requested. The logic is pretty much 1-1 between the DarkModeController and the script in the head... I tried everything that I could come up with to avoid adding anything to the document head, but without it there is always a FOUC when the page reloads into a dark mode context. I'm pretty sure in the current state we could probably do without the DarkModeController fully and still achieve the desired experience. |
There was a problem hiding this comment.
Hey Jason, this is looking good. I pulled it locally and did some testing.
I think we have to live with the FUOC. When I set my system mode to dark and the app mode to light then I see a brief black flash when I do a full page refresh, and the reverse is true: system mode to light and app to dark.
I can't think of any way to avoid this because there's always a split second before the javascript executes. Fortunately, rails Turbo really minimizes it because all the subsequent pageviews are replacements to they don't exhibit the flash.
I tried it with and without the code you have in the and I couldn't tell any difference. Both exhibit a brief flash so I think we should remove the code from the head and just leave it in the stimulus controller. In theory I would expect the flash to be slightly worse in the stimulus controller since it surely executes a little bit later in the render cycle, but in my testing in dev both were really fast flashes and I couldn't tell a difference.
I left a couple really small comments in the code but there is one bug I found. Look at this. When my system setting is on dark and the app is on light a few styles within the app are still stuck in dark mode (notice the conversation selected color on nav bar and the body text of chat messages):
I took a quick look at the source code and I don't understand why it's just these couple styles, it's super weird! One hunch I thought was a tailwind bug with compound modifiers (e.g. dark:group:text-white) but the message body one is not that way, it's really simple: text-gray-950 dark:text-gray-100 Maybe it has to do with tag ordering and one class overriding another somehow differs in this dark/light override state?
Co-authored-by: Keith Schacht <[email protected]>
…ode method to use element instead of documentTarget * fix(application.html.erb): remove data-dark-mode-target attribute
|
@krschacht It turns out that daisyui may be contributing more to the style/layout of the app than you'd likely intended. I tried retain only the necessary css, but that ended up breaking quite a lot. so, instead I've pulled in all of the css, removed the cdn link from the head, removed the styles that were conflicting with our dark mode styles, and minified the daisy css. (...but it looks like the toast is now showing signs of other conflict/regression...) |
|
revisiting your comment regarding daisy ui and 'including a TON of stuff from Daisy and barely using it'. the node + tw plugin should allow you to 'purge unused styles' and ultimately reduce file size. |
|
@jasonpaulso Do you think that's the right path? node is such a big dependency to add. I've been reluctant to add it just for a small thing like this. I think this is only the 2nd time I've found myself wanting to use a tailwind plugin to get some functionality, and the other time I worked around it. I would lean towards, instead, just removing the whole Daisy minified include and copy out the ~6 or so classes which are actually being used from Daisy (for things like drop-down menu). That's probably out of scope for this PR so if that's the route to go then we can merge this in knowing there are a couple dark/light mode visual quirks if you leave "system" mode and we can remove Daisy in a follow-up PR. We already get purging of unused styles for Tailwind itself so we don't need for that it's only for Daisy. What do you think? You're more in the weeds on this. |
…fers dark matches, but light mode is prefered. * chore(daisy_selected.css): remove daisy_selected.css * chore(_head.html.erb): re-add daisy link_tag
@krschacht after having a good look around (and reading comments you've made in various related discussions on the internets, incidentally) I can agree with your point. I think this issue is mainly a daisy thing and doing some local testing we still encounter some of the same conflicts even with daisy included via node_modules. The current commit on this PR just reapplies the daisy "light" theme when the user has the preference of "light" in the app, but the system/browser happens to be dark—it's passing the tests and I think things look good otherwise! |
There was a problem hiding this comment.
Jason, nice job chasing down those issues. I pulled it locally and went through all 4 scenarios and it's so close! I just found a few more small issues but I suspect these will be easy fixes:
-
When I've forced my user setting to dark mode or light mode, all the colors look correct! But when I change my user setting to "system" then switch OS to Dark, the selected state for conversations is off.
-
I created a new user account so that the color preference is unset. When I go to user settings, I confirm this by noticing that none of the radio buttons are selected. When I toggle my OS light/dark the app doesn't change. I think we should officially make the default user preference be "system" both so that the OS light/dark toggles the app and when you go to user settings you see "system" radio selected even though you never set anything. I think the fix for this is to add to user.rb
def preferences
self.preferences.with_defaults(dark_mode: :system)
end
I did not actually test it, but I think that's the right syntax and would fix both issues. If that does fix it, it's worth a couple unit tests just so we don't regress this in the future (e.g. preference defaults to system yet user can overwrite to something else).
- When going to settings and changing the dark mode radio button, can you connect the label to the radio so it's clickable too? I think you just need to add
for: "user_preferences_dark_mode_#{value}"on the user_field.label. While you're in there, I notice the circle radio and the label are not perfectly centered vertically, the circle is floating a little bit below the center line. If that's an easy fix, it would be nice, but if that's tricky we can live with it.
Co-authored-by: Jason Schulz <[email protected]>
* fix(_form.html.erb): add gap between flex items in dark mode color theme section * test(user_test.rb): remove dark_mode preference from boolean values test, add test for default and updating dark_mode preference
…und colors * feat(styles): add support for dark mode in tailwind CSS classes * fix(styles): update tailwind CSS classes for hover background colors in dark mode * fix(views): update tailwind CSS classes for assistant and conversation components
* fix(daisyui_tooltip.css): fix background color and remove unnecessary color property
There was a problem hiding this comment.
@jasonpaulso I saw you pushed changes so I tried this out. When I just now tested this out I saw you fixed (2) and (3) and it looks like you committed a fix for (1) but I was still experiencing it in one case. You fixed it so that forced dark mode was highlighted correctly but now system-enabled dark mode was not. Craziness. But seeing how you were approaching this I knew just what to do so I did a commit with one last tweak.
It's now good to go so I'm going to merge in! Thanks for sticking with this PR until the end! :) It ended up being quite a bit trickier than I expected it to be. I thought it would just be a change to the /settings page.
|
And I’m not done yet. I went through several scenarios today and I’ve got more revisions to come. Maybe they’ll compliment your change!
… On Apr 30, 2024, at 5:37 PM, Keith Schacht ***@***.***> wrote:
@krschacht approved this pull request.
@jasonpaulso <https://github.com/jasonpaulso> I saw you pushed changes so I tried this out. When I just now tested this out I saw you fixed (2) and (3) and it looks like you committed a fix for (1) but I was still experiencing it in one case. You fixed it so that forced dark mode was highlighted correctly but now system-enabled dark mode was not. Craziness. But seeing how you were approaching this I knew just what to do so I did a commit with one last tweak.
It's now good to go so I'm going to merge in! Thanks for sticking with this PR until the end! :) It ended up being quite a bit trickier than I expected it to be. I thought it would just be a change to the /settings page.
—
Reply to this email directly, view it on GitHub <#296 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACT5ZUXEGKC7XWU5AXF7P2TZAAFIHAVCNFSM6AAAAABGTRSFH2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZSGU3DGOJXGQ>.
You are receiving this because you were mentioned.
|
|
Oh shoot! Sorry I rushed that out. :( Push it to a new PR and feel free to
change what I did if it causes any complications. It was a tiny change.
On Tue, Apr 30, 2024 at 4:41 PM Jason Schulz ***@***.***>
wrote:
… And I’m not done yet. I went through several scenarios today and I’ve got
more revisions to come. Maybe they’ll compliment your change!
> On Apr 30, 2024, at 5:37 PM, Keith Schacht ***@***.***> wrote:
>
>
> @krschacht approved this pull request.
>
> @jasonpaulso <https://github.com/jasonpaulso> I saw you pushed changes
so I tried this out. When I just now tested this out I saw you fixed (2)
and (3) and it looks like you committed a fix for (1) but I was still
experiencing it in one case. You fixed it so that forced dark mode was
highlighted correctly but now system-enabled dark mode was not. Craziness.
But seeing how you were approaching this I knew just what to do so I did a
commit with one last tweak.
>
> It's now good to go so I'm going to merge in! Thanks for sticking with
this PR until the end! :) It ended up being quite a bit trickier than I
expected it to be. I thought it would just be a change to the /settings
page.
>
> —
> Reply to this email directly, view it on GitHub <
#296 (review)>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ACT5ZUXEGKC7XWU5AXF7P2TZAAFIHAVCNFSM6AAAAABGTRSFH2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZSGU3DGOJXGQ>.
> You are receiving this because you were mentioned.
>
—
Reply to this email directly, view it on GitHub
<#296 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAIR5MXVEQ4GYNIL37CZBTZAAFYHAVCNFSM6AAAAABGTRSFH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBXGQZTAMJTHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
