React

[branlok]

Small contribution to button component for better UI/UX and development [#1879].

Goals:

• To improve the button design system for better UI/UX and branding with Kitsu.

Changes proposed in this pull request:

• Button Component API now provides 4 variants + configurables(color, padding)
• Added Outline variant - act as secondary button
• Added Borderless variant - act as tertiary button
• Added Screen variant - button for distracting backgrounds.
• Reintegrated primary variant
• Removed Inverted
• Additional description, configurable, defaults, and examples to Storybook

File notes

• /Button/index.tsx - logic for api and useLayoutEffect for patching button local css var()
• /Button/styles.module.css - logic for style changes button behaviour
• /themes/light.css + /dark.css - added more a list of colors

/cc @NuckChorris @Reinachan

[branlok]

with the latest commit e27d41d

• Added a react custom hook with mutation observer to detect theme changes.*
• fixed default colour set to green for primary button
• fixed borderless text colour match what is provided in alternativeColors.

*This hook can be reused for components that just needs to bind current theme to more complicated styling configuration. One can write out all possible styling into global css space, but i think encapsulate the logic within the component makes things more maintainable. While css space should use sparingly with base colours and/or default values.

[Reinachan]

I feel like you're overcomplicating things a bit when it comes to colouring which ends up giving the button too much responsibility. The button shouldn't be aware of what theme it's being displayed in. That responsibility should be on the colour variables we set at the root. I also feel like your approach to changing colours isn't particularly clean either. I'd suggest you try to use classes for alternative colours like you're doing with other things already.

[branlok]

I feel like you're overcomplicating things a bit when it comes to colouring which ends up giving the button too much responsibility. The button shouldn't be aware of what theme it's being displayed in. That responsibility should be on the colour variables we set at the root. I also feel like your approach to changing colours isn't particularly clean either. I'd suggest you try to use classes for alternative colours like you're doing with other things already.

Thank you honestly, I definitely overcomplicated it 😅 😅 . Without going too far why, I basically made some mistake and misunderstood something with postcss that left me producing some unsatisfactory code which went against DRY principles. So, I went for an alternative to encapsulate the logic in the component which felt reasonable coming from someone whom used a lot of css-in-js.

But anyways, upon backtracking of my stumbles, I did solved my misunderstanding - and I've converted most of the JS code to module.css

[NuckChorris]

I don't doubt that we actually have both these behaviors throughout the site, but I wonder if we should unify on one or the other for consistency? 🤔

[branlok]

Thanks for taking the time to review. Yep, we can continue to tailor them down.

I design them with the intention to bring more headroom for some ordered customizability. The idea was to always use default/fallback values as means to maintain consistency i.e. omitted hoverBehaviour prop will default to 'bg-darken'. Like any optional props here, they are customizable that should be used situationally when needed .

😅 tho I agree nonetheless, I think more nuance customization do come with more understanding and responsibility to use appropriately. hover-behaviour may just be too much, so yes, unify on one. I'll declutter it, and keep our core options, i.e. size/color/padding/etc. Better to expand accordingly as opposed to over-preparing .

[NuckChorris]

Is there some way we can eliminate this conditional logic from the button stylesheet? Ideally components should not have to think about themes, they should just reference logical variables (not color variables) from the theme stylesheet, to keep the theming all in one place

If you remove the bg-darken and bg-lighten that might make it easier 🤔

[branlok]

Yep, removing bg-darken/lighten property will make it easier to rid of those conditional logic, tho they can be moved to global stylesheet as well. But i will follow thru with the first review, remove them.

[NuckChorris]

What is meant by "safe" in this context?

[branlok]

the "safe" keyword are colours I checked were safe to use against white/dark backgrounds, since I was reusing the colour palette already defined. I didn't want to alter those colours nor add new ones in case they were important. Thus I added two new colours, both deviated slightly for better readability.

currently they should be the same value across all three themes, and it works well. but if needed extra contrast - one can always alter to liking. the original palette for the two specific colours just didn't do it for buttons. especially for something like borderless/outline variants.

[NuckChorris]

That makes a lot of sense. I have been noticing a lot of cracks in the current color palette approach and thinking of maybe copying what github does and having a set of numbered shades (1-7) for each color. For now this safe thing works and we can clean it up when we figure out a better palette system 😅

[NuckChorris]

Sorry for the delay, I had a bunch of stuff come up at my day job. Couple points I'd like to address before we merge still, but this is legitimately one of the best PRs I've ever seen 😍

[NuckChorris]

I think my biggest remaining question is if -100 and -200 are the best naming scheme, since they don't really say what the colors are for. In fact, I don't fully grasp what they're for. Looks like active and hover states? Would -hover and -active work?

[branlok]

mhm, yeah good suggestion. i've changed the variable names to -hover and -active, for a better association and clarity. I forgotten, but now I also removed the 'darken' variables, since the last request for better uniformity.