-
Notifications
You must be signed in to change notification settings - Fork 54
BREAKING: Button - implement dark theme and match redlines #336
Conversation
affects light and dark
removing some unused button theme named colors
Codecov Report
@@ Coverage Diff @@
## master #336 +/- ##
=======================================
Coverage 89.33% 89.33%
=======================================
Files 64 64
Lines 1256 1256
Branches 186 163 -23
=======================================
Hits 1122 1122
Misses 131 131
Partials 3 3 Continue to review full report at Codecov.
|
Lovely to see this one being worked on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove secondaryTinted button type and replace it with a customization example where the variables are used to create this custom usage. This way, the API is not required by all themes.
…x/buttons-darktheme
src/components/Button/Button.tsx
Outdated
@@ -34,7 +34,7 @@ export interface IButtonProps { | |||
onFocus?: ComponentEventHandler<IButtonProps> | |||
renderIcon?: ShorthandRenderFunction | |||
text?: boolean | |||
type?: 'primary' | 'secondary' | |||
type?: 'primary' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really want to remove for all the Stardust clients? What is the point to have type
being a string with secondary
removed - it should be a boolean
flag then. However, would disagree that we should remove secondary
from the list of types provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "secondary" mean though, vs just a "default" button? Or conversely, if we have a "primary" and "secondary" button visual treatment, then the "neither primary nor secondary button" becomes a liability (in the context of Teams at least) because there is no such thing as a button that isn't primary or secondary... do we make "primary" and "secondary" non-optional for button? That seems onerous.
I agree that perhaps "primary" should be a Boolean rather than a string... This might also be useful in "groups" of buttons, to make some sort of rule that breaks if there is more than one primary...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets agree on whether secondary
should be removed as type
enum value for Button
@levithomason
…x/buttons-darktheme
affects light and dark
Button
API Change
"type" prefix removed from buttonVariables:
Before
After
(For general reference, hover, pressed, keyboard focus states not shown)