feat(ui): add ui-components package
#7223
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RedYetiDev
force-pushed
the
basic-ui-package
branch
from
b7a1cae
to
8ec0d3f
Compare
|
Any idea why Vercel is failing? |
|
Maybe create a package stories ? |
cc @ovflowd and @bmuenzenmeyer |
|
FYI I'm drafting until the Next.js update is done. Feel free to review, this just can't land until that does (given that it modifies the |
RedYetiDev
force-pushed
the
basic-ui-package
branch
from
8ec0d3f
to
84b26ff
Compare
RedYetiDev
force-pushed
the
basic-ui-package
branch
from
84b26ff
to
2e3dc50
Compare
|
Hey, can someone let me know why Vercel is failing? |
| import { | ||
| STORYBOOK_MODES, | ||
| STORYBOOK_SIZES, | ||
| } from '../../../.storybook/constants'; |
There was a problem hiding this comment.
FYI Storybook should exist only within the UI components package :) -- not within the root of the repository.
There was a problem hiding this comment.
Storybook is in use in both the UI components, and the app/site. To prevent duplicate code, I moved it to the repo root (like Tailwind), what do you have in mind?
There was a problem hiding this comment.
Which components are on app/site that use Storybook? IMO, Storybook should ONLY exist for the UI components package.
Because semantically, that's what it is supposed to be used for. For describing components. I think we use it on apps/site only for visual regression, for whose I don't think they should be storybooks but just snapshots. We should probably make that change before we make this package then.
There was a problem hiding this comment.
We can still use Chromatic for that: https://www.chromatic.com/features/visual-test.
IMO you can do that on this PR, decoupling stories from website-only components (which actually are combination of the base components) and simply use Cypress or Node.js or whatever for the visual regression tests with Chromatic.
There was a problem hiding this comment.
I agree with this overall, I just don't think that this should be done now. Currently, while only a subset of features are available in the UI package, I think it's okay to share a storybook config, as both the UI package and the apps/site package ned it. Later on, once more stuff is moved into the package, the storybook should only exist there, just not yet. Not while only the icons have been transferred.
But, in the future, this is probably the best direction to go in.
There was a problem hiding this comment.
The current Storybook configuration in /apps/site differs from the one in packages/ui-components because the ui-components package doesn't rely on Next.js internally. If we were to move the Storybook setup into ui-components now, we'd have to introduce Next.js dependencies, as the configuration in apps/site depends on Next.js internals. The goal of this package is to not have Next.js dependencies.
A more effective approach IMO would be to gradually migrate components to the packages/ui-components package while removing Next.js-related elements from their Storybook configurations. Once all configurations are free of Next.js and consolidated in packages/ui-components, transferring the entire Storybook setup to this package will be much smoother.
By "Next.js-free," I mean the removal of dependencies like next-intl, Link, and similar Next.js-specific elements.
There was a problem hiding this comment.
The current Storybook configuration in /apps/site differs from the one in packages/ui-components because the ui-components package doesn't rely on Next.js internally.
That doesn't make sense. The current Storybook configuration is Next.js-agnostic.
There was a problem hiding this comment.
If we were to move the Storybook setup into ui-components now, we'd have to introduce Next.js dependencies, as the configuration in apps/site depends on Next.js internals. The goal of this package is to not have Next.js dependencies.
Could you elaborate where you saw that? I'm pretty sure it is not.
There was a problem hiding this comment.
By "Next.js-free," I mean the removal of dependencies like next-intl, Link, and similar Next.js-specific elements.
You do know these lines are to "mock" components that use such dependencies, and that the ui-components library does not use these said dependencies, since the Link component for example, will now use Dependency Injection... Also, the Storybook should only exist for UI components, meaning there is nothing Next.js specific; I'm not sure where the confusion is here.
Sorry, but I'm adamant that Storybook should only be for ui-components, I won't approve this PR without that :/
There was a problem hiding this comment.
A more effective approach IMO would be to gradually migrate components to the packages/ui-components package while removing Next.js-related elements from their Storybook configurations. Once all configurations are free of Next.js and consolidated in packages/ui-components, transferring the entire Storybook setup to this package will be much smoother.
I don't think we need a gradative migration on the main branch; as mentioned before, nothing stops you from making a feature branch and gradatively migrating the components; there's no rush to getting this done, and I doubt that conflicts will arise over time...
I'll defer to the rest of the @nodejs/nodejs-website if they are fine with merging this, as it will complicate things further with two different Storybooks.
I only want that we have a dedicated Storybook for UI components and that's it; No other Storybook, not mess needed. I won't block this PR, but I'm expressing my discontent.
| require('@tailwindcss/container-queries'), | ||
| ], | ||
| } satisfies Config; | ||
| export { default } from '../../tailwind.config'; |
There was a problem hiding this comment.
IMO tailwind config should be part of the UI components package as a base tailwind config and we should be able to import it on the Website package.
There was a problem hiding this comment.
This also means that font loading should be done within the UI components package instead on Next.js-side.
Meaning, we should stop using next-fonts and use something that is pure react-ish and that can integrate natively with Tailwind.
There was a problem hiding this comment.
Meaning this package (ui-components) should also export a base CSS file that is something like:
/* global.css */
@tailwind base;
@tailwind components;
@tailwind utilities;
/* update this to use the two fonts we use */
@import url("https://fonts.googleapis.com/css2?family=Poppins:wght@400;500&display=swap");
There was a problem hiding this comment.
Wow, I'm not a fan of what you're proposing. Why does next-font exist, it's to prevent the user of the page from depending on google. but if we do what you suggest that's what's going to happen. Our user will have to go through google. We would therefore need to add a file download system to the building time. To note that googleapis is blocked by not badly of proxy http in company or other thus this user would not have the font.
There was a problem hiding this comment.
I agree with @AugustinMauroy. It's not ideal for the users to need to fetch the font from an external source. If we really wanted to get rid of next-font, we could store the font file as an asset, but even if that was the direction we were going to do in, it should be it's own discussion, and not part of this PR (IMO).
There was a problem hiding this comment.
It is important to mention that all that next-fonts does is download the font during build-time and bundle it within our assets. It still imports the font the same way with a @import url
There was a problem hiding this comment.
There are a couple of issues with next-fonts and why it doesn't work for us:
- We cannot use
net-fontsin the UI components package. That package is agnostic and should only require TailwindCSS and React.- The fonts are essential for the UI components; without the right font families, our Storybooks and Visual Regression tests will have no meaning.
- We shouldn't rely on Next.js to have custom fonts within our Application.
next-fontsis just an easy one-liner abstraction that is nice for most simple projects, but not here.
Our user will have to go through google.
Our servers will anyway need to go through Google, and Google Fonts is also used everywhere across the internet. The chances that these fonts will already be cached by the time they need to request them from our website (at least Open Sans) reduce already the extra request they need to make to our infrastructure to load the font.
Why does next-font exist, it's to prevent the user of the page from
Just because something exists doesn't mean we need to use it.
There was a problem hiding this comment.
I misunderstood what you were initially asking. I thought you meant to get rid of next-fonts for the entire site. Now I understand that you mean only for this package. Yes, you are right, I'll update the CSS file.
There was a problem hiding this comment.
I actually did mean for everything. There's no point of having next-fonts if the ui-components package already loads the fonts 🤷
There was a problem hiding this comment.
Otherwise we are double loading fonts.
|
Hey @ovflowd. I've responded to some of your concerns, but I still have my reservations about the storybook / tailwind concerns, PTAL when you get a chance, thanks! |
|
Thanks @RedYetiDev I've replied to your points ;) |
RedYetiDev
force-pushed
the
basic-ui-package
branch
from
d9b73e3
to
54bd537
Compare
|
The deployment was successful!! |
|
@RedYetiDev maybe you're unfamiliar with the website workflows, but we add a |
|
Lighthouse Results
|
github-actions
bot
removed
the
github_actions:pull-request
Unit Test Coverage Report
Unit Test Report
|
| @@ -0,0 +1,7 @@ | |||
| @import 'https://fonts.googleapis.com/css2?family=IBM+Plex+Mono:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;1,100;1,200;1,300;1,400;1,500;1,600;1,700&family=Open+Sans:ital,wght@0,300..800;1,300..800&display=swap'; | |||
There was a problem hiding this comment.
FYI don't forget to add a preconnect on the Next.js base layout.
| @import 'tailwindcss/base'; | ||
| @import 'tailwindcss/components'; | ||
| @import 'tailwindcss/utilities'; | ||
| @import '../../../apps/site/styles/base.css'; |
There was a problem hiding this comment.
It feels weired that the ui components package depends on another package's styles (the website) -- that should definitely not be the case. These styles should live here if nothing else.
There was a problem hiding this comment.
(But I see this is only for storybook, so 🤷)
This PR moves the first few components into the
ui-componentspackage.For now, only the
Icons/folder has been moved, as it's quite simple to move (unlike some other components), and it gives a good basis of what this package will look like.Over coming PRs, I'll slowly move more pieces into this package.