Dark mode implementation #167

LucianPetri · 2022-12-30T00:53:37Z

Dark Mode Implementation Details Here: #90

…mentation

k-nearest-neighbor · 2022-12-30T01:28:36Z

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.

fozziethebeat · 2022-12-30T01:29:26Z

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

AbdBarho · 2022-12-30T08:35:54Z

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

index.tsx was not intended to be part of previous commit

k-nearest-neighbor · 2022-12-30T16:44:53Z

website/src/pages/index.tsx

@@ -21,12 +21,12 @@ const Home = () => {
          content="Conversational AI for everyone. An open source project to create a chat enabled GPT LLM run by LAION and contributors around the world."
        />
      </Head>
-      {session ? (
+      {false ? (


This debugging hack snuck in to my commit. Will be setting it back to session.

fozziethebeat

This looks pretty close to being ready. What's stopping us from handling the merge conflicts and checking it in as is?

fozziethebeat · 2023-01-01T01:51:10Z

website/src/styles/Theme/components/Container.ts

+  }
+}
+
+export const containerTheme = defineStyleConfig({


I'd suggest naming the variable the same as the thing being themed. In this case Container

In the index.ts we can then do

export { default as Container } from "./Container";

This is how Chakra structures their pro themes.

fozziethebeat · 2023-01-01T03:12:26Z

website/src/styles/Theme/index.ts

@@ -0,0 +1,29 @@
+import { type ThemeConfig, extendTheme } from "@chakra-ui/react";
+import { containerTheme } from "./components/Container";


Rather than import each components theme here can we create a website/src/styles/Theme/components/index.ts file.

With that we can simplify this as

import * as components from "./components";

And then down below we don't need to define a const components = { ...}. We can just use the import

fozziethebeat · 2023-01-01T04:56:02Z

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.

…nused 'ColorModeSwitch' component for now.

k-nearest-neighbor · 2023-01-02T02:19:43Z

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:

chakra-ui/chakra-ui#6905
chakra-ui/chakra-ui#6028 (comment)

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

…onent, which is for contianing width (and buggy). Last commit before pulling in latest to resolve merge confilicts, and continue to fix the survey pages.

k-nearest-neighbor · 2023-01-03T04:31:42Z

Alright. All checks passing and ready.

fozziethebeat

Great to finally get this in.

I'll make some small targeted fixes for things in a follow up PR

LucianPetri added 18 commits December 29, 2022 18:40

Add Color Mode

721012a

Basic Color switch - WIP

ded14c7

UserMenu darkMode refactor

8a4855c

Footer darkMode refactor

186eeb7

Auth darkMode Refactor

2431a52

reorganizing the theme configuration

c204fbd

implementing scaling of theme

fc64fd6

discarding ColorModeScript

b8f3fe9

adding default text Color to Container

a537a0f

text Color

dcdcade

refactor Header

ae58be5

Refactor Footer

5f4d8ac

remove unused import

851e73f

prettier colorSwitch WIP

8bb2a3c

adding a cubic bezier for transition

80d2e02

adding darkmode to pages

e5a1cad

Merge https://github.com/LAION-AI/Open-Assistant into dark-mode-imple…

84805dd

…mentation

post merge pages darkMode

5d719e2

LucianPetri requested review from fozziethebeat, k-nearest-neighbor and AbdBarho as code owners December 30, 2022 00:53

LucianPetri linked an issue Dec 30, 2022 that may be closed by this pull request

Implement dark mode on website #90

Closed

15 tasks

LucianPetri added 2 commits December 30, 2022 03:13

fixing eslint

4d53bbe

pre-commit lint

53673d2

fozziethebeat added website UI/UX All website Components labels Dec 30, 2022

fozziethebeat assigned LucianPetri Dec 30, 2022

k-nearest-neighbor added 2 commits December 30, 2022 08:36

Fix chakra adding extra padding on container.

a98d4f3

Reset debugging var

8b7e9bd

index.tsx was not intended to be part of previous commit

k-nearest-neighbor reviewed Dec 30, 2022

View reviewed changes

fozziethebeat reviewed Jan 1, 2023

View reviewed changes

k-nearest-neighbor added 2 commits January 1, 2023 15:57

border-bottom should be configurable and off for transparent

862a0ac

Add an icon-based toggle. Fix alignment of items in header. Keeping u…

6299f36

…nused 'ColorModeSwitch' component for now.

Theme implementation which does not rely on the Chakra Container comp…

97625a8

…onent, which is for contianing width (and buggy). Last commit before pulling in latest to resolve merge confilicts, and continue to fix the survey pages.

AbdBarho mentioned this pull request Jan 2, 2023

Use Chakra UI for Messages #264

Merged

k-nearest-neighbor and others added 4 commits January 2, 2023 00:49

Merge branch 'main' into dark-mode-implementation

5f3d32b

Darkmode for survey pages. Fixes for eslint.

3d13c7c

Merge branch 'main' into dark-mode-implementation

de7e8c0

fix method signature from merge. eslint fixes

a76441b

one more fix for header border-bottom. Also fixed some eslint whine

e546eaa

fozziethebeat approved these changes Jan 3, 2023

View reviewed changes

fozziethebeat merged commit a926703 into main Jan 3, 2023

fozziethebeat deleted the dark-mode-implementation branch January 3, 2023 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dark mode implementation #167

Dark mode implementation #167

LucianPetri commented Dec 30, 2022

k-nearest-neighbor commented Dec 30, 2022

fozziethebeat commented Dec 30, 2022

AbdBarho commented Dec 30, 2022

k-nearest-neighbor Dec 30, 2022

fozziethebeat left a comment

fozziethebeat Jan 1, 2023

fozziethebeat Jan 1, 2023

fozziethebeat commented Jan 1, 2023

k-nearest-neighbor commented Jan 2, 2023 •

edited

k-nearest-neighbor commented Jan 3, 2023

fozziethebeat left a comment

		@@ -0,0 +1,29 @@
		import { type ThemeConfig, extendTheme } from "@chakra-ui/react";
		import { containerTheme } from "./components/Container";

Dark mode implementation #167

Dark mode implementation #167

Conversation

LucianPetri commented Dec 30, 2022

k-nearest-neighbor commented Dec 30, 2022

fozziethebeat commented Dec 30, 2022

AbdBarho commented Dec 30, 2022

k-nearest-neighbor Dec 30, 2022

Choose a reason for hiding this comment

fozziethebeat left a comment

Choose a reason for hiding this comment

fozziethebeat Jan 1, 2023

Choose a reason for hiding this comment

fozziethebeat Jan 1, 2023

Choose a reason for hiding this comment

fozziethebeat commented Jan 1, 2023

k-nearest-neighbor commented Jan 2, 2023 • edited

k-nearest-neighbor commented Jan 3, 2023

fozziethebeat left a comment

Choose a reason for hiding this comment

k-nearest-neighbor commented Jan 2, 2023 •

edited