Skip to content
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

[ButtonBase] Better keyboard focused story #11090

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 21, 2018

  • The TypeScript definitions where assuming the classes object was cascading between the different elements. It does not! I took a strong implementation bias by not allowing it.
    The CSS is already cascading, having the same behavior for the classes property would have made debugging much harder. I believe I have fixed all the definitions cc @pelotom

Breaking change

  • Rename the keyboardFocused feature focusVisible in order to follow the CSS specification wording:
    https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo
  • Give up on the classes property to host the focus visible feature. The fact that the classes don't cascade was making it hard to use. Instead, we rely on a focusVisibleClassName property. This is allowing any component along the rendering chain to use the feature. For instance, a Switch component: Switch > SwitchBase > IconButton > ButtonBase.
<ButtonBase
- classes={{
-   keyboardFocused: 'my-class-name',
- }}
+ focusVisibleClassName="my-class-name"
/>

Closes #10976

@oliviertassinari oliviertassinari force-pushed the solve-keyboard-focused-state branch from 3225237 to be4a964 Compare April 21, 2018 18:12
@oliviertassinari oliviertassinari merged commit 1d9e576 into mui:v1-beta Apr 21, 2018
@oliviertassinari oliviertassinari deleted the solve-keyboard-focused-state branch April 21, 2018 18:24
Copy link
Member

@pelotom pelotom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants