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.
| 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 should be it's own discussion, and not part of this PR (IMO).
So I think we need to find a solution for this that doesn't depend on Google and then look for the most refined solution.
| <link rel="preconnect" href="https://fonts.googleapis.com" /> | ||
| <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin /> | ||
|
|
||
| <link |
There was a problem hiding this comment.
There was a problem hiding this comment.
tailwind.config.ts
Outdated
There was a problem hiding this comment.
|
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! |
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.