Open-Assistant icon indicating copy to clipboard operation
Open-Assistant copied to clipboard

Dark mode implementation

Open LucianPetri opened this issue 2 years ago • 5 comments

Dark Mode Implementation Details Here: #90

LucianPetri avatar Dec 30 '22 00:12 LucianPetri

This is a great start. There are a few areas that need to be addressed:

  • UI regressions: Changes to light mode theme; Header functionality and appearance
  • UI that isn't incorporated into dark mode yet / well.
  • Aesthetics.

I'm going to go through this PR and highlight these, probably committing.

k-nearest-neighbor avatar Dec 30 '22 01:12 k-nearest-neighbor

This is a pretty big PR touching a lot of files. Let's hold off merging other react component changes until this is reviewed and we're comfortable with it (or deciding some other resolution strategy).

fozziethebeat avatar Dec 30 '22 01:12 fozziethebeat

I also agree that this is a great start. thank you @LucianPetri.

I would like to propose a strategy for moving forward with this:

Ideally, all dark mode handling should be done either be configuring the theme directly, or by having some conditional styling for the components under /components only. This should also include font-size, shadows, gaps, padding, etc...

Everything under /pages should only care about what is being shown, not its design, by using the aforementioned components.

This should make our work future proof, and changes to the design both easy and global.

I am not sure if browser support is there yet, or if chakra ui supports it, but Container queries sound really promising for paddings margins and such...

AbdBarho avatar Dec 30 '22 08:12 AbdBarho

I took a look at this branch. I think we should just check this in as is and then revise the small UI issues in later (tinier) PRs.

The key things to fix before approval:

  • Fix the merge the conflicts above
  • Make sure the action boxes in tasks all are visible. Right now rank_assistant_replies is visible and looks great. The others done (i'm guessing because of how that box is styled). I think the fix is as simple as copy whatever is done in rank_assistant_replies to the other tasks.

fozziethebeat avatar Jan 01 '23 04:01 fozziethebeat

The UI and implementation issues actually aren't minor. The implementation uses Container to provide the theme. The purpose of Container is to constrain width, not to provide theme, so it's a problematic mixing of concerns, and adds a lot of unneeded wrappers everywhere. Chakra Container also has a bug where it imparts padding which is impossible to fully remove. See:

https://github.com/chakra-ui/chakra-ui/pull/6905 https://github.com/chakra-ui/chakra-ui/issues/6028#issuecomment-1367979340

On top of that, there are UI regressions in the light theme home page, and header, and the dark theme is broken / not implemented for the grading pages. So the dark theme needs a lot of work.

We should have a typical merge policy where merges must:

  • Not have regressions (UI or otherwise)
  • Be "done" wherever possible
  • i.e., no breaking trunk

I have fixes for these things though. I think this should be ready to merge in a few hours (dinnertime now 🍲). Currently working on:

  • implementation that doesn't use Chakra Container to provide the theme
  • icon toggle
  • header fixes
  • Some styles
  • rebase to fix merge conflicts

k-nearest-neighbor avatar Jan 02 '23 02:01 k-nearest-neighbor

Alright. All checks passing and ready.

k-nearest-neighbor avatar Jan 03 '23 04:01 k-nearest-neighbor