feat: extend ShadCN initialization #13347

TylerAPfledderer · 2024-07-07T06:50:17Z

Description

Extends #13343 in initializing the setup of Radix/Tailwind (ShadCN)

Add the tailwind prettier plugin
Adds a set of locally structured primitive components from Radix (generated via ShadCN cli)
Adds all foundational tokens and global styles to tailwind config
- Prefers CSS vars to be able to cleanly create semantic tokens for color mode
Updates Storybook config with a reusable provider decorator containing the Chakra and next-themes providers
- This decorator is also used in prod to improve sync with storybook
Contains a tailwind folder holding an accordion and stack components.
- Currently used for example with migration, but are not currently active in prod (full migration to be done in a separate PR)

TylerAPfledderer · 2024-07-07T19:13:56Z

tailwind/ui/__stories__/accordion.stories.tsx

+
+export default meta
+
+export const Basic: StoryObj = {


Found complication here.

Passing the type of Accordion (which is actually Accordion.Root from Radix) to the meta object ends up with the args option being given the type never, though it should be required in this setup.

Seems like the type of this component is pretty complex and StoryObj might not be acknowledging it and assuming it's not a component type.

TylerAPfledderer · 2024-07-07T19:57:01Z

src/styles/main.css

-      --ring: 0 0% 83.1%;
-    }
+@layer components {
+  .accordion-trigger {


Either dump all the utility classes used for a primitive part into a single custom class name here, or have the utilities directly declared in the component file.

Though what to do about readability here with components that have variants? See tailwind/ui/button.tsx

Now that I removed the button component here, take a look at the source file

@TylerAPfledderer didn't get why we are doing this approach. Could you elaborate more?

Don't understand why we are not using the same form as in Shadcn: just using the utility classes on the component. I think it is much clearer and easier to tweak, right?

I was just presenting this approach to you for inquiry of opinion.

If we have all the utilities under a class like I have here, then all those utilities will not render separately when viewing dev tools, so the styling is easier to read and digest because all the styling would still be listed under that one class name.

The downside here is that we are separating general styling from the way we would create variants, such as the approach ShadCN would take in the button component.

I don't have a problem either way and can personally manage the readability if all the styling was kept directly at the component. The prettier plugin should also provide the ability to sort the order of the utility classes used.

Oh I see now. Ok, that is interesting to know.

I would say that, at least for this initial stage, lets keep with the "tailwind/shadcn" convention since all the documentation/examples are pointing in that direction (I think it will be clearer for most devs and contributors) and because I see those downsides that you mention as important. IMO the DX having this separated files is going to be worse than using the classes on the component itself where you can easily find and modify them.

The other minor (but important to us at this point) is performance. Defining new classes will increase the size of the generated css as you are not reusing the utility classes on the compiled bundles.

I'll merge this PR as it is and we can tweak these things later.

pettinarip

Amazing @TylerAPfledderer, thanks!

Couple of questions.

I see that you have created a completely new folder in which you are putting the new stories for the Shadcn components. This basically creates two separate SBs instances. I was thinking earlier about catching regressions with Chromatic while we do the migration, could be really helpful. With this approach, I don't think we'll be able to have that, right? Wouldn't it be possible to use the same SB config and support both? Chakra and Shadcn components? or do you think it is going to be a mess to config and maintain that?
Noticed that you imported a bunch of the Shadcn components right away. I'm not 100% sure if we're going to use all of them at this point. I was thinking that it might be better to install them as we migrate things, so that at the end we only have the necessary components installed. I.e. the pagination component, I doubt we will be using that. I believe that if we leave it in, we are just never going to clean it up.

pettinarip · 2024-07-08T06:51:42Z

.prettierignore

@@ -4,3 +4,4 @@ yarn.lock
 package-lock.json
 public
 build
+!tailwind.config.js


why is this needed? (on the other hand, its a .ts, .js)

Whoops! Had issues earlier with prettier. This should be removed

pettinarip · 2024-07-08T06:53:49Z

.prettierrc

-}
+  "trailingComma": "es5",
+  "plugins": [
+    "prettier-plugin-tailwindcss"


Nice! very handy

pettinarip · 2024-07-08T06:57:29Z

src/lib/utils/**/*.ts

🤔 hmm why this file name/route? don't like it honestly. I think we can make it simpler by just following the conventions we have with the other utils, or am I missing something? are you expecting to add more things in here?

I understand. I frankly don't really understand how defining the alias in components.json is different than defining it in the tsconfig.

I am not anticipating other utilities to be created, but since the cn() is in it's own file in a utils folder instead of a utils file, I figured that would be the better approach.

I still go back to my initial comment above though 😅

I still don't get the idea of what this files does haha. Can we remove it since it is currently duplicating the cn function?

Not sure if this is adding a benefit or not.

Are you talking about confusion with the components.json file? Used for configuration when adding components via the cli

No, sorry. I'm taking about the filename **/*.ts. On the base branch, I've already created a cn.ts file. Was wondering if we could remove one to avoid the duplication.

I frankly don't really understand how defining the alias in components.json is different than defining it in the tsconfig.

Me neither xD

pettinarip · 2024-07-08T07:12:00Z

src/styles/main.css

Do you like the main.css file? was thinking that perhaps we could have one of these two options:

main.css and fonts.css

global.css and fonts.css

main and global at the same time seems very redundant to me.

I would want to follow the provided convention. global.css and font.css make sense here.

pettinarip · 2024-07-08T11:56:46Z

tailwind.config.ts

-	],
+    "./src/**/*.{ts,tsx}",
+    // TODO: remove after migration
+    "./tailwind/**/*.tsx",


Ok, you plan on moving the ui to the components folder once the migration is done?

I would expect to do this if we are going to follow the suggested convention provided by ShadCN. It makes sense to me for the "Atom" components at least.

pettinarip · 2024-07-08T12:01:22Z

tailwind.config.ts

+          highContrast: "var(--primary-highContrast)",
+          lowContrast: "var(--primary-lowContrast)",


Not sure about this. Do you know how these are compiled? are they going to be kebab case? or are they going to keep the same?
This: bg-primary-lowContrast vs bg-primary-low-contrast. I would prefer to keep them as kebab now that we are using this convention. What do you think?

Compiling retains pascalCase. Good with me flipping them to kebab. That feels more natural here.

I've updated the naming here

pettinarip · 2024-07-08T12:50:04Z

tailwind/.storybook/main.ts

+ */
+
+const config: StorybookConfig = {
+  // TODO: Post migration - ui directory should be moved under `src/components`


Got it, this answers my previous question about post-migration tasks.

TylerAPfledderer · 2024-07-08T13:59:46Z

Amazing @TylerAPfledderer, thanks!

Couple of questions.

I see that you have created a completely new folder in which you are putting the new stories for the Shadcn components. This basically creates two separate SBs instances. I was thinking earlier about catching regressions with Chromatic while we do the migration, could be really helpful. With this approach, I don't think we'll be able to have that, right? Wouldn't it be possible to use the same SB config and support both? Chakra and Shadcn components? or do you think it is going to be a mess to config and maintain that?

Noticed that you imported a bunch of the Shadcn components right away. I'm not 100% sure if we're going to use all of them at this point. I was thinking that it might be better to install them as we migrate things, so that at the end we only have the necessary components installed. I.e. the pagination component, I doubt we will be using that. I believe that if we leave it in, we are just never going to clean it up.

@pettinarip

I can experiment here. My initial thought was that it would create a mess, but I can take another pass. My intention here was not to ignore Storybook. I had a different approach, but it would've been more steps.

For the sake of this PR, if it's fine to just update the existing config, then I would not want to allow the new accordion file and its stories to get pushed here, which could start adding or making conflicting snapshot. They would need to be reserved for a new PR that does the full migration of this component for Chromatic and prod.

No problem removing them, and just adding when necessary.

…ation

TylerAPfledderer · 2024-07-08T21:54:18Z

@pettinarip Was able to sync up the provider changes into the main Storybook config.

Two things:

I removed those Radix components except for Accordion, which I made a story for testing. If you'd like, I can stash what I did to add in a separate PR when properly migrating the component for prod. (Thereby removing the need for that tailwind folder)
View the comment I made in the denied UI test for the StatsBoxGrid, trying to figure out what to do about this regression.

TylerAPfledderer · 2024-07-08T22:44:37Z

src/components/ThemeProvider.tsx

+
+  const theme = useMemo(() => merge(customTheme, { direction }), [direction])
+  return (
+    <NextThemesProvider


Having both providers in one reusable component helps with the sync with Storybook and prod.

Also note here that I added the style tag in here with the CSS vars for the fonts for the same reason as above. See that TODO comment also

Looks better 👍🏼

pettinarip · 2024-07-09T10:47:49Z

Two things:

I removed those Radix components except for Accordion, which I made a story for testing. If you'd like, I can stash what I did to add in a separate PR when properly migrating the component for prod. (Thereby removing the need for that tailwind folder)

All good with leaving that on the PR. Serves as an example and future implementations.

TylerAPfledderer · 2024-07-09T13:31:16Z

Two things:

I removed those Radix components except for Accordion, which I made a story for testing. If you'd like, I can stash what I did to add in a separate PR when properly migrating the component for prod. (Thereby removing the need for that tailwind folder)

All good with leaving that on the PR. Serves as an example and future implementations.

Ok! I leave it where it is. Storybook config is not looking at the tailwind folder, and will work on full migration of accordion in a separate PR.

TylerAPfledderer · 2024-07-09T13:53:15Z

@pettinarip should be good to go here?

pettinarip

LGTM @TylerAPfledderer! lfg

TylerAPfledderer added 15 commits July 5, 2024 15:52

feat: initialize shadCN setup

1a5a98c

feat: add next-themes for color mode

1893c0d

refactor(.storybook): update configuration for Tailwind

f30e52f

feat(global.css): set color tokens

d1dde72

refactor(global.css): complete update of foundation styles

b051c50

refactor: update accordion component

9f06e0f

feat: extend shadcn initialization

4a68d63

chore: move tailwind styles to main.css

5656859

revert(components/ui): remove eth- utility prefix

f527b54

refactor: create isolated storybook config for tailwind

c3da52f

chore(tailwind.config): update content paths

d9ec82a

refactor(tailwind/ui/accordion): apply icon from react-icons

0245dd1

chore(tailwind/ui/stories): remove test story

6f80922

chore(components.json): set rsc to false

e351988

chore(tailwind/.storybook): update breakpoint set for viewports

7fcb255

TylerAPfledderer requested review from wackerow, corwintines, pettinarip and minimalsm as code owners July 7, 2024 06:50

github-actions bot added config ⚙️ Changes to configuration files dependencies 📦 Changes related to project dependencies tooling 🔧 Changes related to tooling of the project labels Jul 7, 2024

TylerAPfledderer mentioned this pull request Jul 7, 2024

Shadcn/Tailwind - initial setup #13343

Merged

2 tasks

TylerAPfledderer added 3 commits July 7, 2024 14:28

chore: bump prettier to v3

c8d89cd

refactor: update next theme provider usage

d0e4956

fix(tailwind/stories): fix type errors for accordion

d07e666

TylerAPfledderer commented Jul 7, 2024

View reviewed changes

chore: remove carousel component ui

c790d14

TylerAPfledderer commented Jul 7, 2024

View reviewed changes

fix(tailwind/ui): fix accordion collapse animation

e95298b

feat(tailwind/ui): create stack components

675492d

pettinarip reviewed Jul 8, 2024

View reviewed changes

TylerAPfledderer added 6 commits July 8, 2024 14:43

Merge branch 'shadcn-initial-setup' into feat/shadcn-extend-initializ…

24ca478

…ation

chore(.prettierignore): remove tailwind config

a680d58

refactor(.storybook): update root conifg decorator

7236038

chore: remove generated radix components

a066d37

fix(HubHero): fix RTL position of content container

7ee19ac

revert: add back radix nav menu package

94e4d1a

TylerAPfledderer commented Jul 8, 2024

View reviewed changes

TylerAPfledderer added 2 commits July 8, 2024 22:30

chore: rename css files

91b9185

chore(global.css): flip pascalCase naming to kebab-case

4fcc57c

TylerAPfledderer mentioned this pull request Jul 9, 2024

refactor: use @font-face import for mono font #13364

Merged

pettinarip approved these changes Jul 11, 2024

View reviewed changes

pettinarip merged commit 547dffb into ethereum:shadcn-initial-setup Jul 11, 2024
5 of 6 checks passed

TylerAPfledderer deleted the feat/shadcn-extend-initialization branch July 11, 2024 12:35

TylerAPfledderer mentioned this pull request Jul 15, 2024

feat: migrate text and headings to tailwind #13386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extend ShadCN initialization #13347

feat: extend ShadCN initialization #13347

TylerAPfledderer commented Jul 7, 2024 •

edited

Loading

TylerAPfledderer Jul 7, 2024

TylerAPfledderer Jul 7, 2024 •

edited

Loading

TylerAPfledderer Jul 9, 2024

pettinarip Jul 10, 2024

TylerAPfledderer Jul 10, 2024

pettinarip Jul 11, 2024

pettinarip Jul 11, 2024

pettinarip left a comment

pettinarip Jul 8, 2024

TylerAPfledderer Jul 8, 2024

pettinarip Jul 8, 2024

pettinarip Jul 8, 2024

TylerAPfledderer Jul 8, 2024

TylerAPfledderer Jul 8, 2024

pettinarip Jul 9, 2024

TylerAPfledderer Jul 9, 2024

TylerAPfledderer Jul 9, 2024

pettinarip Jul 10, 2024

pettinarip Jul 8, 2024

TylerAPfledderer Jul 8, 2024

pettinarip Jul 8, 2024

TylerAPfledderer Jul 8, 2024

pettinarip Jul 8, 2024

TylerAPfledderer Jul 8, 2024

TylerAPfledderer Jul 9, 2024

pettinarip Jul 8, 2024

TylerAPfledderer commented Jul 8, 2024

TylerAPfledderer commented Jul 8, 2024 •

edited

Loading

TylerAPfledderer Jul 8, 2024

pettinarip Jul 9, 2024

pettinarip commented Jul 9, 2024

TylerAPfledderer commented Jul 9, 2024 •

edited

Loading

TylerAPfledderer commented Jul 9, 2024

pettinarip left a comment

		highContrast: "var(--primary-highContrast)",
		lowContrast: "var(--primary-lowContrast)",

feat: extend ShadCN initialization #13347

feat: extend ShadCN initialization #13347

Conversation

TylerAPfledderer commented Jul 7, 2024 • edited Loading

Description

Choose a reason for hiding this comment

TylerAPfledderer Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettinarip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerAPfledderer commented Jul 8, 2024

TylerAPfledderer commented Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettinarip commented Jul 9, 2024

TylerAPfledderer commented Jul 9, 2024 • edited Loading

TylerAPfledderer commented Jul 9, 2024

pettinarip left a comment

Choose a reason for hiding this comment

TylerAPfledderer commented Jul 7, 2024 •

edited

Loading

TylerAPfledderer Jul 7, 2024 •

edited

Loading

TylerAPfledderer commented Jul 8, 2024 •

edited

Loading

TylerAPfledderer commented Jul 9, 2024 •

edited

Loading