search

VEuPathDB / CoreUI

Core UI for VeuPathDB applications. Provides components, style definitions, and utilities to enable developers to rapidly assemble complex applications with consistent UI and UX across our portfolio of sites.
1 stars 0 forks source link

Update Switch to new Toggle component #101

Closed chowington closed 2 years ago

chowington commented 2 years ago

Works toward https://github.com/VEuPathDB/web-eda/issues/1252 Fixes https://github.com/VEuPathDB/CoreUI/issues/104

jernestmyers commented 2 years ago

Just another suggestion to perhaps document the SwitchStyleSpec types that surfaced while reviewing related work.

chowington commented 2 years ago

Documentation would be nice for sure. I think we can also at least make some minor changes to make it more clear without having to rely on the docs

chowington commented 2 years ago

Updates made based on last styling meeting. Review for review!

chowington commented 2 years ago

If we're going that route, should we model it after the existing CoreUI Paragraph component or even just use its style spec directly? Or are we not assuming even in our label style spec that the label is just text?

chowington commented 2 years ago

If we're going that route, should we model it after the existing CoreUI Paragraph component or even just use its style spec directly? Or are we not assuming even in our label style spec that the label is just text?

@jernestmyers @asizemore Any thoughts on this above?

asizemore commented 2 years ago

I thought we wanted to allow the label be any element so that it could include a tooltip, with the use case being the starred variables toggle? Or did we cover that case differently and i just don't remember?

chowington commented 2 years ago

I thought we wanted to allow the label be any element so that it could include a tooltip, with the use case being the starred variables toggle? Or did we cover that case differently and i just don't remember?

That's true, Ann! This comment is about what kind of styleOverrides we should provide for the label. Here's the full thread (a bit scattered around the PR):

Jeremy: I think Dave made a good point about the fontSize of the label for small versus medium toggles. I'm not sure if the best approach is to add an optional prop specifying the label size, or to handle it as a styleOverride.

Connor: I think given the fact that we're changing the label to be a ReactNode, we can just allow the user to provide their own custom node if they need to customize the label. What do you think?

Jeremy: Regarding the point about the label, it feels like providing a style override to handle its font size would be more in line with CoreUI but I'm not certain. To that end, I guess we'd have to consider what styleOverrides are appropriate for a label: margins/padding, color, size?

Connor: If we're going that route, should we model it after the existing CoreUI Paragraph component or even just use its style spec directly? Or are we not assuming even in our label style spec that the label is just text?

jernestmyers commented 2 years ago

I think I'm okay with the label's styleOverride mimicking the Paragraph component.

asizemore commented 2 years ago

Lol thank you for the recap @chowington ! I had lost track 😆 so that was very helpful! I think either route is great. I do like the simplicity (from CoreUI's perspective) of putting the burden on the user for including styling with their ReactNode. However, using the same styleOverrides as Paragraph offers visual consistency and consistency in the code.

I think the comments would just need to be clear that if a user passes in a ReactNode for the label as well as styleOverrides for it, then the styleOverrides will be used instead of whatever style is there.

Actually i like that lol i'm now leaning towards the styleOverrides-matching-Paragraph route :)