Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the design system color #1628

Open
bjohansebas opened this issue Sep 21, 2024 · 9 comments
Open

Set the design system color #1628

bjohansebas opened this issue Sep 21, 2024 · 9 comments
Assignees
Labels
enhancement UI Change, update, or fix for site UI (not content)

Comments

@bjohansebas
Copy link
Member

It would be great to start using CSS variables to maintain consistency in colors. I've been trying to do this in previous PRs, but I think it's time to define it completely. This way, we could simplify dark mode and solve several issues with !important.

Inspired by how shadcn/ui handles colors, I'm going to leave a list of colors we could use:

Light mode:

--bg: #ffffff (background)
--fg: #383838 (general texts)
--card-bg: #f0f1f3 (header, code blocks, announcement, etc)
--card-fg: #383838
--hover-bg: #484848 (hover background)
--hover-fg: #fafafa (hover foreground)
--border: #d9e1e4 (borders)
--accent: #0e78ce (Links)

image

Dark mode:

--bg: #0c0c0c
--fg: #fafafa (general texts)
--card: #181818 (header, code blocks, announcement, etc)
--card-foreground: #71717A
--hover-bg: #484848 (hover background)
--hover-fg: #fafafa (hover foreground)
--border: #d9e1e4
--accent: #2b8fe0 (Links, hover background)

image

cc/: @expressjs/docs-wg

@bjohansebas bjohansebas added the UI Change, update, or fix for site UI (not content) label Oct 26, 2024
@chrisdel101
Copy link
Contributor

Are we changing the colors too? For dark mode I pulled almost all the colors from github's dark mode. Are we changing darkmode colors for changes sake only?
Or because we're not satisfied with the ones we have? If it's the first reason, I suggest we leave them. I could be convinced otherwise though if there is a reason :)

I agree with the rest of the stuff. Maybe we can integrate scss here too? If it's not too much work that is. Makes systems like this easier to maintain. Plain CSS is also fine if not.

If no one is working on this I can start.

@bjohansebas
Copy link
Member Author

bjohansebas commented Oct 27, 2024

It is not necessary to change all the colors of the dark mode, just to improve a bit the contrast between the colors. They look fine on a computer, but on mobile, they are almost the same (it also depends on the screen). However, if you don’t want to, that’s okay. What I do want is to put all the colors in css variables.

image

Maybe we can integrate scss here too? If it's not too much work that is. Makes systems like this easier to maintain. Plain css is also fine if not.

I have never used it, and I don't know how it would integrate with Jekyll. I preferred that we continue using plain css. In the end, css has evolved quite a bit and now has several features that scss has (nesting and variables, I think, are the most important), and browser support has improved significantly.

@chrisdel101
Copy link
Contributor

I didn't know you could nest in CSS! This was one of scss's main uses, plus it has functions. it look like jekyll supports it, but I'm good with plain CSS.

but on mobile, they are almost the same (it also depends on the screen).

If you've encountered it then we can try out the colors you put above.

@chrisdel101
Copy link
Contributor

chrisdel101 commented Nov 20, 2024

Do we want the variable in hex or RBG? I'd prefer all vars to be one or the other so we can compare them easier. In order I prefer names when possible, hex, then RGB. Hex okay?

@bjohansebas
Copy link
Member Author

I prefer hex

@chrisdel101
Copy link
Contributor

chrisdel101 commented Dec 27, 2024

Hey @bjohansebas, just wanted to check in about this. I got a bit off track maybe? I decided to move the light mode to it's own file where all the colors are variables. So light and dark mode each have their own file now.
Why? It will easier to pinpoint where styles are coming from. I'm finding alot or unused of overwritten rules along the way here.

The css vars are named like these examples.
Why? Its much easier to search for styles when they have distinct names IMO, rather than naming them the same just under different scopes. The vars are in a file called values.css and is just imported when needed.

  --lightm-bg: #ffffff;
  --lightm-fg: #383838;
  --darkm-fg: #fafafa;
  --darkm-card-bg: #181818;
....

The PR has tons of bit changes and will be hard to review, even though it's not difficult stuff, so I can prob break apart into separate PRs. Are you okay with this separation or themes? It leaves the styles.css the same expect for the colors. The work is around 50% as it's very time consuming.
If yes, I'll continue, and propose how to break the PRs up for reviewing.
If no, then I'll move the light-mode.css back into the main file (but hopefully it's okay).

@bjohansebas
Copy link
Member Author

I think that, even though reviewing everything in a single PR might be more complicated, in this case, it would be the best option.

I decided to move the light mode to it's own file where all the colors are variables

I'm okay with this separation. Regarding the variable names, I had thought of using the same names to avoid writing more code, just changing the variable with a class in the style of shadcn/ui, so I would lean towards following that approach.

During these holidays, I've been thinking a bit about the future of the website. Next year, I really want to finally make expressjs/discussions#211 a reality, so we can stop worrying about style-related matters. However, this will involve a series of discussions that might take some time. So, I'm not sure whether we should leave this as it is for now or continue. @chrisdel101 what do you think?

@chrisdel101
Copy link
Contributor

So u mean not continuing with this enhancement at all? If yes, it’d say it’s a pity since I’ve spend a lot of time, but I also get that’s how it goes sometimes.
If it’s going to all get thrown out later anyway then maybe we close it? On the other hand, it would make life much easier having a color scheme until the big change occurs.

IMO we move forward with it due to amount of work already done. But fine with me either way.

@bjohansebas
Copy link
Member Author

It's true, we should have a color scheme until this can happen, because it might still be needed in the future. And since you already have a large part of that work done, I'm fine with you continuing. Anyway, we'll have to coordinate on the other issue, and that will take time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement UI Change, update, or fix for site UI (not content)
Projects
None yet
Development

No branches or pull requests

2 participants