Refactor/button v1

[JaneMoroz]

Description

I've recently came across clsx + twMerge (to efficiently merge Tailwind CSS classes in JS without style conflict) and CVA. So I created ButtonCVA with some variants (basically one for color, one for size, but I think we can create one for horizontal allignment and maybe try to style an icon inside it too, just to avoid using custom styles at all... well if we want).

I used this Figma component as a base.

So this just something to consider, we might want to use smth like this or not. If we go for some UI library, we might not need it at all, even though I think clsx + twMerge combination is kinda cool. Current input/textarea components can use smth like this, we have a lot of conditional styling there.

Issue link

Fixes # (issue)

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
chingu-dashboard	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 3, 2023 1:25pm

[Dan-Y-Ko]

I do like this but there's a couple things. Like you mentioned, if we decide to use another ui component library, this is probably not needed. I think we should decide what we want to use. Second, I think with how the figma currently is, this would be more annoying because all the sizes are written in px in the figma, but I can see if that's a quick fix for design team in figma, to reference the size variants you mapped out here.

There is something I need clarification on. Is there still going to be custom styles attached to the button for things outside of the color and size? I'm guessing that's where classname comes in.

Another thing. I'm guessing the size is for the font size? What about font weight?

[JaneMoroz]

I do like this but there's a couple things. Like you mentioned, if we decide to use another ui component library, this is probably not needed. I think we should decide what we want to use. Second, I think with how the figma currently is, this would be more annoying because all the sizes are written in px in the figma, but I can see if that's a quick fix for design team in figma, to reference the size variants you mapped out here.

sizes already come from figma, but we should surely make buttons a bit responsive because they seem kinda big.

There is something I need clarification on. Is there still going to be custom styles attached to the button for things outside of the color and size? I'm guessing that's where classname comes in.

Just add classes in 'className' and styles will be rewritten nicely (or better say merged idk) thanks to tailwind-merge

Another thing. I'm guessing the size is for the font size? What about font weight?

according to figma, font-weight is 600 (I think for every size but I need to double check) so I included font-semibold in general button (before variants) but again maybe we need to add some breakpoints and decrease it so it can look better on small desktop screens

[Dan-Y-Ko]

according to figma, font-weight is 600 (I think for every size but I need to double check) so I included font-semibold in general button (before variants) but again maybe we need to add some breakpoints and decrease it so it can look better on small desktop screens

I guess my main question is if it needs to be extended to add a font weight, is that easily doable? I haven't looked at the details of how this works yet. It looks like it should be easy enough to do.

Another question, does this specifically have to be a regular dependency (instead of dev dependency)? Considering Tailwind is a dev dependency, I don't see why this can't be.

[JaneMoroz]

according to figma, font-weight is 600 (I think for every size but I need to double check) so I included font-semibold in general button (before variants) but again maybe we need to add some breakpoints and decrease it so it can look better on small desktop screens

I guess my main question is if it needs to be extended to add a font weight, is that easily doable? I haven't looked at the details of how this works yet. It looks like it should be easy enough to do.

easy, just add any classes to className. For example right now in <Button/> component, not in <ButtonCVA/> we have grid and I can't rewrite it because without tailwind-merge merging tailwind classes is a lottery. With tailwind-merge you know for sure that className is the last source of truth.
For example, even though all buttons have justify-center, you can simply add justify-start and the button's content will be aligned to the left:

<Button
  type="button"
  variant="secondary"
  size="md"
  className="justify-start"
>
   Add Tech Stack
</Button>

The same can be done with font-weight.

Another question, does this specifically have to be a regular dependency (instead of dev dependency)? Considering Tailwind is a dev dependency, I don't see why this can't be.

idk, probably

[Dan-Y-Ko]

Can you fix the conflicts. This is good to merge after that.