-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
feat(ui): add ui-components package #7401
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
95d6983
to
f3ddf1e
Compare
No need to rush. Jest still work. Also we need to work step by step. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
f3ddf1e
to
4f6e508
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates generic UI components from the website codebase to a new, centralized package (@node-core/ui-components) while updating related documentation, workflows, and Storybook configurations. Key changes include:
- Migrating and updating component imports (e.g. ActiveLink, Preview, icons) to use @node-core/ui-components.
- Updating documentation (COLLABORATOR_GUIDE.md) and workflows (.github/workflows/lint-and-tests.yml) to reflect the new package structure.
- Removing legacy Storybook configurations and unused component stories from apps/site.
Reviewed Changes
Copilot reviewed 290 out of 296 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
apps/site/components/Common/ActiveLink.tsx | Updated to wrap the generic BaseActiveLink from the new @node-core/ui-components pkg. |
COLLABORATOR_GUIDE.md | Documentation updated to reference the new package and file locations. |
.github/workflows/lint-and-tests.yml | Workflow now targets packages/ui-components for building Storybook and coverage. |
apps/site/app/[locale]/layout.tsx | Updated global styles import to use the centralized Tailwind configuration. |
apps/site/app/[locale]/next-data/og/[category]/[title]/route.tsx | Icon imports updated to use components from @node-core/ui-components. |
apps/site/components/Blog/BlogPostCard/index.tsx | Updated the Preview component import to reference the new package location. |
apps/site/components/Blog/BlogPostCard/tests/index.test.mjs | Adjusted BlogPostCard import to reflect file movement within apps/site. |
apps/site/.storybook/main.ts | Removed Storybook configuration, in favor of the configuration now in the new package. |
apps/site/components/Blog/BlogHeader/index.stories.tsx | Removed legacy Storybook stories corresponding to migrated components. |
apps/site/components/Common/AvatarGroup/* | Removed Avatar, AvatarGroup, and related Storybook files as they’re no longer needed. |
Files not reviewed (6)
- apps/site/.stylelintignore: Language not supported
- apps/site/components/Common/AvatarGroup/Avatar/index.module.css: Language not supported
- apps/site/components/Common/AvatarGroup/Overlay/index.module.css: Language not supported
- apps/site/components/Common/Badge/index.module.css: Language not supported
- apps/site/components/Common/Banner/index.module.css: Language not supported
- apps/site/components/Common/Blockquote/index.module.css: Language not supported
What?! A successful deployment on the first try?! Woo! |
Lighthouse Results
|
Storybook failed to deploy because storybook has been moved in this PR, could a repo admin manually deploy and test? Thanks! |
packages/ui-components/Common/AvatarGroup/Avatar/index.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR updated the workflow for the new package, I added a workflow_dispatch trigger to run the modified workflow manually. (As the
pull_request_target
trigger is setup to only run on themain
branch)
Ah I see, please remove that afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through all the code. Took me a while. I just wanted to thank you for the massive work done here! It is immensively appreciated, Aviv :)
Yep, will do! That'll be the last thing done, as (as you can see above) the storybook won't deploy unless via that manual trigger. |
…into feat/ui-package
View Storybook / Check Chromatic Tests Tests@nodejs/website
Coverage: 87% @node-core/ui-components
Coverage: 95% |
@ovflowd I've addressed your reviews. This PR passes all tests and lints, but it requires a manual merge because the workflow for running Storybook has changed in this PR, and the old workflow isn't compatible with this package. |
Unless someone objects, I'm planning to merge this at the end of the day (UTC-4) on Monday (let's say around 8pm). That should give reviewers time to take a look and object if need be. Feel free to 👎 this comment, and I'll wait longer. |
All good. Feel free to do a manual merge, or ping me if blocked. |
I'm not quite sure how one would install this package outside the monorepo (i.e. from |
I do believe we can provide git paths within the npm install command |
Third-party tools like GitPkg allow you to do this, but I'm not sure how we feel about using a third-party tool for installing dependencies. However, both yarn (yarnpkg/yarn#4725 (comment)) and pnpm (pnpm/pnpm#7487) support this. We would need to convert api-docs-tooling to a yarn or pnpm project, which would mean that any project installing it would also need yarn / pnpm, and I'm not how core will feel about using something other than npm for api generation (Although maybe this is an issue for api-docs-tooling) |
I've opened nodejs/api-docs-tooling#236 for further discussion. |
@node-core/ui-components
Overview
This PR introduces the
@node-core/ui-components
package, which centralizes generic UI components used across the website. Previously, these components were located inapps/site/components/{Common,Containers}
.What Has Changed?
@node-core/ui-components
.apps/site/components/{Blog,Downloads}
as they are tailored for the website.Implementation Details
Prop-Drilling Strategy
To keep
@node-core/ui-components
framework-agnostic, it does not rely on Next.js. However, many components on the website do depend on Next.js for functionalities such as routing and translations.To maintain this separation, Next.js-specific dependencies are passed via props instead of being accessed directly within components.
Example: Migrating a Next.js-dependent Component
Original implementation in
apps/site
:New implementation:
@node-core/ui-components
(agnostic version)apps/site
(Next.js dependencies injected as props)How to Use These Components?
apps/site
.@node-core/ui-components/<path/to/component>
.This ensures that
@node-core/ui-components
remains decoupled from Next.js while still allowing seamless integration with the website.Storybook Integration
apps/site
. Instead, it is now a (dev) dependency of@node-core/ui-components
.__design__
stories have been moved@node-core/ui-components
.npm run dev
or check the stories of components they depend on.Tailwind Configuration
Tailwind remains a dependency in both
apps/site
and@node-core/ui-components
. However, the Tailwind configuration has been centralized within@node-core/ui-components
.Key Changes:
@node-core/ui-components
.apps/site
, so no additional configuration is needed inapps/site
.@config
variable in the CSS file, removing the need to re-exportKnown Issue: Font Configuration
--font-open-sans
--font-ibm-plex-mono
Additional Changes
FNM Logo Update
CSS
styles/
have been moved into@node-core/ui-components
.Logo Renames
@node-core/ui-components
package have new names:Twitter
->X
Discord
's default export was calledBluesky
, it is nowDiscord
.Next Steps
While setting up
@node-core/ui-components
, two TODO comments were added for future improvements:Navbar Styles Refactor (
apps/site/withNavBar.tsx
)@node-core/ui-components/Containers/NavBar/index.module.css
.Tailwind Font Configuration
@node-core/ui-components
fully framework-agnostic.Requests for Reviewers
.github/workflows/lint-and-tests.yml
has been updated to reflect this change. Could someone verify that the changes were correct?TL;DR
✅ Moves generic UI components to
@node-core/ui-components
✅ Moves Storybook dependency from
apps/site
to@node-core/ui-components
✅ Removes Storybook stories for non-migrated components
✅ Centralizes Tailwind configuration
✅ Moves global styles into the new package
✅ Updates FNM logo with the correct vector
🔜 Future work needed on two TODO comments
Thank you all for being patient over the past few months as this slowly comes together.