search

allyourbot / hostedgpt

An open version of ChatGPT you can host anywhere or run locally.
MIT License
192 stars 75 forks source link

Dark mode: allow user to set it #296

Closed jasonpaulso closed 2 weeks ago

jasonpaulso commented 3 weeks ago
jasonpaulso commented 3 weeks ago

@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.

jasonpaulso commented 3 weeks ago

It seems like most of the inscrutable issues you're mentioning are related to the way DaisyUI handles dark mode. I've managed to override one part of it by redefining the "bg-base-100" color, which is used in the hover menus. Another aspect is the text color. In my quest for a solution, I found some helpful information, specifically this discussion. By installing, requiring, and configuring the DaisyUI plugin, I was able to avoid its conflicts and render our styles as defined. While preparing and researching a justification for doing so, I found this exchange and decided to hold off. However, all this effort paid off in another way; I managed to eliminate our need for both a new dark mode controller and a script in the head. This last bit of documentation did not clearly describe what Tailwind's darkMode configuration options were capable of, in my opinion.

krschacht commented 3 weeks ago

@jasonpaulso dang, it’s worth mentioning that i’m not sold on Daisy UI so we could rip it out. I believe there are only 2-3 components we are using and I’ve had in mind that after a little while if the use of Daisy has not grown then we could simply copy & paste the styles for those 3 components rather than inheriting the whole library.

In other words, we’re including a TON of stuff from Daisy and barely using it, so if we’re fighting the framework…

jasonpaulso commented 3 weeks ago

@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...)

jasonpaulso commented 3 weeks ago

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.

krschacht commented 3 weeks ago

@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.

jasonpaulso commented 3 weeks ago

@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.

@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!

jasonpaulso commented 2 weeks ago

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 https://github.com/allyourbot/hostedgpt/pull/296#pullrequestreview-2032563974, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACT5ZUXEGKC7XWU5AXF7P2TZAAFIHAVCNFSM6AAAAABGTRSFH2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZSGU3DGOJXGQ. You are receiving this because you were mentioned.

krschacht commented 2 weeks ago

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 < https://github.com/allyourbot/hostedgpt/pull/296#pullrequestreview-2032563974>, 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 https://github.com/allyourbot/hostedgpt/pull/296#issuecomment-2087430139, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIR5MXVEQ4GYNIL37CZBTZAAFYHAVCNFSM6AAAAABGTRSFH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBXGQZTAMJTHE . You are receiving this because you were mentioned.Message ID: @.***>