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] Fix missing keyboard ripple #8723

Merged
merged 1 commit into from
Oct 17, 2017
Merged

[ButtonBase] Fix missing keyboard ripple #8723

merged 1 commit into from
Oct 17, 2017

Conversation

sakulstra
Copy link
Contributor

@sakulstra sakulstra commented Oct 16, 2017

Hopefully this is the fix for #8438

Olivier edits:

I have changed my mind. I don't think that the pulsation display should be the default behavior for the autoFocus property. It feels too opinionated. For instance, it's not how a native input behaves.
I would rather see people opting-in this behavior with #3008.

Still, I have found a bug with the keyboard display of the ripple. We are good now:

Closes #8438

@oliviertassinari oliviertassinari added the component: button This is the name of the generic UI component, not the React module! label Oct 16, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I'm not sure it's going to be enough. I can't see the pulsating mode of the ripple navigating to the Button documentation page. I'm wondering what's missing 🤔.

@@ -41,6 +41,10 @@ type ProvidedProps = {

export type Props = {
/**
* If `true`, the ripple will me active on mount
Copy link
Member

Choose a reason for hiding this comment

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

Missing dot at the end.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Oct 16, 2017
@sakulstra
Copy link
Contributor Author

sakulstra commented Oct 16, 2017

@oliviertassinari ah i added start() instead of pulsate() - should pulsate now :)

but i have to investigate how this is working internally - i think there is still sth wrong with keyboard focus

@oliviertassinari
Copy link
Member

Nop, that won't make it, the pulsation state needs to be in sync with the focus state. I'm looking into it.

event.persist();
const keyboardFocusCallback = this.onKeyboardFocusHandler.bind(this, event);
detectKeyboardFocus(this, this.button, keyboardFocusCallback);
// Fix for https://github.com/facebook/react/issues/7769
Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari changed the title [Button] autoFocus ripple on render [ButtonBase] Fix missing keyboard ripple Oct 17, 2017
@oliviertassinari oliviertassinari merged commit 7c6fae8 into mui:v1-beta Oct 17, 2017
the-noob pushed a commit to the-noob/material-ui that referenced this pull request Nov 17, 2017
@tiagowippel
Copy link

tiagowippel commented Nov 17, 2017

It doesn´t working yet.. even in the demos. https://material-ui.com/demos/dialogs/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants