Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

feat(css-modules): initial version of css modules #2676

Merged
merged 9 commits into from
Aug 28, 2022

Conversation

ovflowd
Copy link
Member

@ovflowd ovflowd commented Aug 26, 2022

Description

This PR moves all the styles into CSS modules and removes unused components, styles and fixes other styles.

Related Issues

  • #1468

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run lint:js -- --fix and/or npm run lint:md -- --fix for my JavaScript and/or Markdown changes.
    • This is important as most of the cases your code changes might not be correctly linted
  • I have run npm run test to check if all tests are passing, and/or npm run test -- -u to update snapshots if I created and/or updated React Components.
  • I have checked that the build works locally and that npm run build work fine.
  • I've covered new added functionality with unit tests if necessary.

@ovflowd
Copy link
Member Author

ovflowd commented Aug 26, 2022

Note.: With this PR the /docs layout will be partially broken. The idea here is anyways to ignore the docs page for now as we're revisiting it and refactoring it in the near future.

@benhalverson benhalverson added the create-preview Generate preview on staging.nodejs.dev label Aug 27, 2022
@github-actions github-actions bot removed the create-preview Generate preview on staging.nodejs.dev label Aug 27, 2022
@github-actions
Copy link

Please find a preview at: https://staging.nodejs.dev/2676/

@ovflowd
Copy link
Member Author

ovflowd commented Aug 27, 2022

Update:

  • Finished all the migration to CSS modules and even more cleanup
  • Now double-checking CSS optimisations and removing redundant things, renaming class names for simpler ones and double-checking components interfaces, style imports and stuff.
  • Double checking that the style is still working as intended
  • Then finally going to update the tests and finally add tests to the components that don't have ones.

@ovflowd ovflowd added enhancement New feature or request create-preview Generate preview on staging.nodejs.dev labels Aug 27, 2022
@github-actions github-actions bot removed the create-preview Generate preview on staging.nodejs.dev label Aug 27, 2022
@github-actions
Copy link

Please find a preview at: https://staging.nodejs.dev/2676/

@ovflowd ovflowd marked this pull request as ready for review August 27, 2022 18:43
@ovflowd
Copy link
Member Author

ovflowd commented Aug 27, 2022

Update:

  • Finished all previous tasks
  • Only need to add some more tests and double-check all existing ones

Would gently ask a review from @benhalverson @rodion-arr @manishprivet @mikeesto

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2022

Codecov Report

Merging #2676 (b0f3878) into main (1fcda83) will decrease coverage by 0.74%.
The diff coverage is 83.97%.

@@            Coverage Diff             @@
##             main    #2676      +/-   ##
==========================================
- Coverage   88.25%   87.51%   -0.75%     
==========================================
  Files         100      100              
  Lines         996      977      -19     
  Branches      276      252      -24     
==========================================
- Hits          879      855      -24     
- Misses        110      115       +5     
  Partials        7        7              
Impacted Files Coverage Δ
src/components/Banner/index.tsx 100.00% <ø> (ø)
...wnloadAdditional/DownloadableItem/downloadItems.ts 100.00% <ø> (ø)
src/components/DownloadAdditional/index.tsx 100.00% <ø> (ø)
src/components/DownloadHeader/index.tsx 100.00% <ø> (ø)
src/components/Dropdown/index.tsx 100.00% <ø> (ø)
src/components/EditLink/index.tsx 100.00% <ø> (ø)
src/components/Hero/index.tsx 100.00% <ø> (ø)
src/components/InstallTabs/LinuxPanel/index.tsx 100.00% <ø> (ø)
src/components/InstallTabs/MacOSPanel/index.tsx 100.00% <ø> (ø)
src/components/Layout/page.tsx 100.00% <ø> (ø)
... and 49 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ovflowd ovflowd added the create-preview Generate preview on staging.nodejs.dev label Aug 27, 2022
@github-actions github-actions bot removed the create-preview Generate preview on staging.nodejs.dev label Aug 27, 2022
@github-actions
Copy link

Please find a preview at: https://staging.nodejs.dev/2676/

@ovflowd
Copy link
Member Author

ovflowd commented Aug 27, 2022

I've noticed that material-icons takes a good amount of bundle, so I'm going to switch from a font-based approach to an SVG one by using https://mui.com/material-ui/icons/#svg-material-icons

@ovflowd
Copy link
Member Author

ovflowd commented Aug 28, 2022

Done, updated to the usage of SVG-only icons. Bundle reduced by around 200KB. And the initial content shifting is also reduced, as the icons don't need to be swapped via font family.

@ovflowd ovflowd added the create-preview Generate preview on staging.nodejs.dev label Aug 28, 2022
@github-actions github-actions bot removed the create-preview Generate preview on staging.nodejs.dev label Aug 28, 2022
@ovflowd
Copy link
Member Author

ovflowd commented Aug 28, 2022

Note.: I also enabled font-aliasing here to remove the sharp edges from fonts; it will give the impression on some screens that the font-weight got reduced, whereas it is just the font-aliasing algorithm that also changes from OS-to-OS and screen-to-screen.

@github-actions
Copy link

Please find a preview at: https://staging.nodejs.dev/2676/

Copy link
Member

@mikeesto mikeesto left a comment

Choose a reason for hiding this comment

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

Great job on this, a huge refactor. I spent some time comparing the preview to the live version and couldn't see any issues.

Copy link
Member

@manishprivet manishprivet left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for working on this

@ovflowd
Copy link
Member Author

ovflowd commented Aug 28, 2022

Thank you, @mikeesto @manishprivet just a heads-up for other minor changes that happened here:

  • Some components were fixed as their styles were mixed with globals causing some confusion, for example, the paddings of the search bar were wrong, the lists on the Navigation bar were wrong, the order of display for the recent-posts on desktop was wrong, and many other small styling artifacts and bugs
  • There are numerous style changes, as you can see; many styles were removed, classes were renamed, and styles were optimised/"simplified" (some were hacky and messy)

I'm going to add tests on a follow-up PR as otherwise this PR will become way too massive.

@ovflowd ovflowd merged commit d8feaaf into nodejs:main Aug 28, 2022
@ovflowd ovflowd deleted the refactor/css-modules branch August 28, 2022 16:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants