Add custom Tailwind themes and a styled Button component#667
Conversation
|
In #648, I will add |
rachelekm
left a comment
There was a problem hiding this comment.
Nice! This is looking really good! My only initial comment is to flesh out the button variants so they align with all button style and size types in the figma system design page.
| orange: "bg-orange-200 text-orange-800 hover:bg-orange-300", | ||
| }, | ||
| size: { | ||
| default: "h-9 px-5 py-2 min-w-9", |
There was a problem hiding this comment.
Instead of default, let's make these entries "large", "medium" and "small", using their respective figma sizing properties. And then for the defaultVariants let's set size to "small" to accomodate mobile-first.
There was a problem hiding this comment.
And then for the defaultVariants let's set size to "small" to accomodate mobile-first.
I thought about it in the first round of development. But after inspecting the buttons in the mobile v.s. desktop designs, I did not find a definitive answer to this. I think maybe setting default to medium makes more sense.
Sometimes a large button is used in the mobile view, for example the finish button; And Sometimes a medium button is used in the mobile view, for example the detail button. large and medium differ in height, x-axis padding, and font size.
There was a problem hiding this comment.
Good observation, medium as the default seems like the way to go!
| variant: { | ||
| primary: "bg-teal-800 text-white hover:bg-teal-950", | ||
| outline: "border border-gray-300 bg-transparent hover:bg-gray-100", | ||
| orange: "bg-orange-200 text-orange-800 hover:bg-orange-300", |
There was a problem hiding this comment.
I was wondering about that too! I saw the secondary button in the button component list, but did not get any luck finding where it is actually used in the mobile or desktop design. So I ended up removing it. I could add secondary back though.
Edit: After a closer look, I found that the "+" and "-" buttons have the same styling as secondary- going to add it back now.
There was a problem hiding this comment.
Thanks! Even if not immediately used, because we're implementing a headless library it might be helpful to implement every variant from the design system page so that if we need to add any other small UI down the road we've already got some form of cohesive styling setup
| List all neighborhoods | ||
| </Button> | ||
| <Button variant="outline"> | ||
| <ArrowLeftIcon className="font-normal h-[18px] w-[18px] text-black" /> |
There was a problem hiding this comment.
I saw you took out the icon sizing in the button styles but I imagine this group of icon class names would be repeated often, do you feel like it's worth keeping out for now? Looks like this would also be handled in your work for #648 so feel free to ignore!
There was a problem hiding this comment.
I removed the icon size button in 4b032b9 because it was a leftover from my experiments. In the mobile or desktop designs, the icon buttons seem to have the same styling as the small, medium, and large buttons. So I suspect we do not actually need a special size for icon buttons. Please correct me if this is not the case though. e.g. those arrow buttons next to "Continue" etc
Edit: Though the "X" button that closes or deletes some stuff seems to have its own styling like below:
There was a problem hiding this comment.
How about this: let's add back icon and only for sizing. We will use outline variant for these X buttons, and then override the background or box shadow etc in the specific places where they are used.
There was a problem hiding this comment.
Thanks for clarifying I misinterpreted that icon size config thinking it affected child component sizing like ArrowLeftIcon and ArrowRightIcon. I think having a separate icon variant makes sense though!
| "css.validate": false, | ||
| "less.validate": false, | ||
| "scss.validate": false | ||
| } |
There was a problem hiding this comment.
Nice, this will be helpful! Maybe let's also add something like the below to the .gitignore so that if anyone has their own personal vs code settings it isn't accidentally committed:
.vscode/*
!.vscode/settings.json
| "react-aria-components": "^1.10.1", | ||
| "react-dom": "^19.1.0", | ||
| "react-router": "^7.6.2", | ||
| "tailwind-variants": "^1.0.0", |
There was a problem hiding this comment.
I haven't used tailwind-variants before but this is interesting and seems really helpful. Interesting that it's made by HeroUI which is one of the component libraries we thought about using and is react-aria + tailwind under the hood!
| {...props} | ||
| // composeRenderProps is a utility from React Aria Components | ||
| // merging a user's className with component-specific classes. | ||
| // This correctly handles all types and states. |
| components: "/src/components", | ||
| pages: "/src/pages", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This will be helpful, thank you!
|
@rachelekm thanks for the review and the suggestions- they are great. I addressed them and update the components page to show different sizes and variants more explicitly. Please let me know if there is any question. |
0d9d762 to
66dd634
Compare
6a32ba2 to
682d1a4
Compare
rachelekm
left a comment
There was a problem hiding this comment.
This looks really great, on the development side this is good to go!
Documenting that we just discussed this component in a huddle and it will be implemented in upcoming work for #648 since that already includes utilizing react-aria's ButtonGroup component:
Oh, I saw this one: Compare favorites in the design system, but did not see it in the designs for mobile or desktop. Did I miss something? Or is it removed from the designs? (I vaguely recall a mention of it, but I am not certain
|
Alright, sorry for how long this took me but hopefully things will go more quickly now that I have the project set up. Looks good overall! I pushed a few small tweaks. Good to go from my POV. |
f37c2e2 to
f22ba16
Compare
|
Thank you both for the reviews and the design tweaks. I squashed my fixup commits and added a changelog entry. Will merge pending CI. |







Overview
This PR:
Checklist
fixup!commits have been squashedCHANGELOG.mdupdated with summary of features or fixes, followingKeep a Changelog guidelines
README.mdupdated if necessary to reflect the changes./scripts/formatto lint, format, and fix the application source code.Demo
Notes
I initially just wanted to define the themes based on Figma design tokens, but then noticed that a good way to test how they work would be creating a styled component, so I took the button component card and included it in the same PR. Changes are separated by commits, so they should be handy to review.
Testing Instructions
Resolves #646
Resolves #647