-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change disabled button style #77
Change disabled button style #77
Conversation
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.
Thanks @brichet
Here are some suggestions
@@ -569,7 +569,6 @@ export const buttonStyles: ( | |||
appearanceBehavior( | |||
'accent', | |||
css` | |||
:host([appearance='accent'][disabled]), |
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.
Is there a specific reason for changing the accent type case?
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.
For consistency.
And maybe we should also remove the background-color
property of :host([disabled])
too.
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.
I would keep it for now as in the opposite of the stealth appearance, the background-color is controlled by the toolkit.
@@ -661,7 +660,6 @@ export const buttonStyles: ( | |||
appearanceBehavior( | |||
'stealth', | |||
css` | |||
:host([appearance='stealth'][disabled]), |
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.
We should remove the all block including on hover and on active case as the goal is to avoid style change when the button is disabled.
/** | ||
* The default disabled opacity. | ||
*/ | ||
export const disabledOpacity = |
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.
Actually as the design token are shared, you should only call withDefault
on the imported token:
import { disabledOpacity } from '@microsoft/fast-components';
export { disabledOpacity } from '@microsoft/fast-components';
disabledOpacity.withDefault(0.3);
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.
Thanks, I didn't know about that.
… avoid creating agin the opacity token
This PR aims to remove the background on disabled buttons, and to change the disabled opacity to 0.4.
The background is visible only in dark mode, and reduce the contrast of the button content.
See jupyterlab/jupyterlab#15021 (comment) and jupyterlab/jupyterlab#15021 (comment) for context.