Skip to content

Design system #1742

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 40 commits into from
Feb 11, 2025
Merged

Design system #1742

merged 40 commits into from
Feb 11, 2025

Conversation

chrisdel101
Copy link
Contributor

@chrisdel101 chrisdel101 commented Feb 3, 2025 •
edited by bjohansebas
Loading

Reallizes #1628.

  • moving colors to variables. Attempting to make colors/names constant across themes.
  • move language css to folder
  • move themes to folder
  • remove unused code them applicable
  • add light mode theme

Left some comments in the codes to note why changes were made. These need deletion before merge.
More commentary on reasons for changes given below.

preview: https://deploy-preview-1742--expressjscom-preview.netlify.app/

Chris Del added 15 commits November 14, 2024 21:37
Adding css to folders
4b9e967 
- theme files and import
Add main vars file + add light mode
4657f25 
- add dark mode announcment icon
- toggle light mode class on/off
- work on vars name and vals
Merge branch 'gh-pages' of https://github.com/expressjs/expressjs.com …
8f73b78 
…into design-system
Add links for lightm and darkm
410fb99 
- add lightm grey side menu color
- add global gray text color
Move all lightm vars out of styles
231ea87 
- mirror moved styles in darkm
- note num vals to new vars
Doctable table head and card
fb096c8 
- lightm lightens the grey
- darkm ligher black
- move global greys to each scope
 - add darkm scroll styles - undo at end
Fix pre plain-text color
3896de8 
- move menu hover boxshadows to themes
- api #menu colors
Move doc boxes to themes
2a3dc6f
blog, eol and support classes
50ba269
Move styles back into styles using same var for both
b2e2922 
- realign into original position
- work on and up to pre tag section
Contiue moving styles back into style.css
4049f3d
Finish moving styles back
11a928e 
- redo nav menus
Move search bar theme to styles
d73c9f3
Remove all items possible from themes to style.css
4762109
Add few more inline comments
23709c5
@netlify Netlify
Copy link

netlify bot commented Feb 3, 2025 •
edited
Loading

✅ Deploy Preview for expressjscom-preview ready!

Name Link
🔨 Latest commit 1c594b5
🔍 Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/67ab74be8b71ad000861ef94
😎 Deploy Preview https://deploy-preview-1742--expressjscom-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

chrisdel101
chrisdel101 commented Feb 3, 2025
View reviewed changes
chrisdel101
chrisdel101 commented Feb 3, 2025
View reviewed changes
chrisdel101
chrisdel101 commented Feb 3, 2025
View reviewed changes
chrisdel101
chrisdel101 commented Feb 3, 2025
View reviewed changes
chrisdel101
chrisdel101 commented Feb 3, 2025
View reviewed changes
@bjohansebas
Copy link
Member

Wow, it looks great, I'll review it very soon

@bjohansebas
Copy link
Member

@chrisdel101 i have fixed the conflicts

@chrisdel101
Copy link
Contributor Author

chrisdel101 commented Feb 3, 2025 •
edited
Loading

@bjohansebas It's still a draft as I'm working fixing a few of the last details, but you can start a review as the remaining things are minor. Overall it's complete, but left as draft due to that.

  • I tried not to make any actual design changes, other than colors, and move things to variables - but there were a few changes that had to happen.
    • redid the overlay as shown above.
    • redid the #top arrow icon (in bottom left) that scrolls to the top of the page - its more pronounce on scroll and on hover.
    • added dark mode 'announcements' icon on index.
    • remove rules that did not exist - used Beautiful soup to search code base for things, and if not found I removed them. I tried to note this where possible with comments.
    • Changed the way the desktop dropdown menu works, and the coloring. Now the top level of the drawer is off when drawer is open and hovering over a child item, and a bottom border was added for clarity on the top level e.g. Guide. I also plan to submit a PR for Submenu navbar anchor active class does not get applied #1726 as I think in this img Guide should be bolded, not Getting Stared (which is bold since it's the page were currently on)

image

  • I'll delete all these "help" type comments that exist, which I left in to help with reviewing i.e. explaining why a change was made.
  • I tried to keep the number of colors to a minimum and so got rid of most of the colors that only occurred once or twice, and replaced with one of the system colors. It should be simple to make any changes to unliked colors, or add in new ones, using this same system. So, things do look different in places.
  • In the values.css I noted which colors got replaced with which - this was mostly just helpful in dev for me, and I will delete these comments before merge.
    I'll add anything else to this block as I complete my PR.

@bjohansebas bjohansebas added design Change, update, or fix for site UI (not content) a11y / accessibility Issues concerning accessibility maintenance Issues/PRs related to making the website maintainable labels Feb 3, 2025
chrisdel101
chrisdel101 commented Feb 4, 2025
View reviewed changes
bjohansebas
bjohansebas reviewed Feb 6, 2025
View reviewed changes
bjohansebas
bjohansebas reviewed Feb 6, 2025
View reviewed changes
bjohansebas
bjohansebas reviewed Feb 6, 2025
View reviewed changes
css/values.css Outdated
@@ -0,0 +1,51 @@
.light-mode:root {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant setting those variables directly in :root to make them even more global.

Suggested change
.light-mode:root {
:root {

Copy link
Contributor Author

@chrisdel101 chrisdel101 Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are scoped. Removing those classes means the styles are not applied to the right themes.
Unless you are just saying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to reiterate - root sets variables globally, and this just scopes it to have a class. I rearranged the order for clarity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we would have to add the light-mode class to the <html> tag, as you initially suggested, since there may be users who disable JavaScript, and the page would look like this:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the light-mode tag back in.

@chrisdel101
Copy link
Contributor Author

Hey @bjohansebas, I made the changes and left comments re: what I did not change. I don't think anything else can be moved rules-wise between files now.

I think I've moved over everything I could now into style.css

bjohansebas
bjohansebas reviewed Feb 9, 2025
View reviewed changes
bjohansebas
bjohansebas reviewed Feb 9, 2025
View reviewed changes
@bjohansebas
Copy link
Member

Great job @chrisdel101! I've left the last two comments, and I can give my approval. btw, I updated the color of the code examples.

@chrisdel101
Copy link
Contributor Author

chrisdel101 commented Feb 10, 2025 •
edited
Loading

@bjohansebas Hey, I made the newest set of requested changes. I added a few comments to the theme JS file here for clarity, but did not change any JS but I changed the names of a few functions (only the names).
Let me know if you find anything else. Thanks for all the help with this review!

bjohansebas
bjohansebas approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm going to wait a day so more people can review it

@chrisdel101
Copy link
Contributor Author

@bjohansebas Just a heads up, I'm going to be away for awhile for medical reasons - maybe like a month. If this branch gets out of date you might need to take over the updates :)

@chrisdel101 chrisdel101 requested review from a team and removed request for a team February 11, 2025 03:33
@bjohansebas bjohansebas merged commit d94ab7d into expressjs:gh-pages Feb 11, 2025
7 checks passed
@bjohansebas
Copy link
Member

@chrisdel101 Great job and take care!

@chrisdel101
Copy link
Contributor Author

@bjohansebas Once again thanks for all your help on this, and for the thorough review.

@chrisdel101 chrisdel101 deleted the design-system branch February 12, 2025 14:21
chrisdel101 added a commit to chrisdel101/expressjs.com that referenced this pull request Feb 16, 2025
@chrisdel101 @bjohansebas
Design system (expressjs#1742)
c3a034c 
Co-authored-by: Sebastian Beltran <[email protected]>
@bjohansebas bjohansebas mentioned this pull request Feb 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlosstenzel carlosstenzel carlosstenzel approved these changes

@bjohansebas bjohansebas bjohansebas approved these changes

Assignees
No one assigned
Labels
a11y / accessibility Issues concerning accessibility design Change, update, or fix for site UI (not content) maintenance Issues/PRs related to making the website maintainable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrisdel101 @bjohansebas @carlosstenzel