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

Fix SpeedDials pressed twice on mobile #10713

Merged
merged 1 commit into from
Mar 19, 2018

Conversation

AlbertLucianto
Copy link
Contributor

@AlbertLucianto AlbertLucianto commented Mar 19, 2018

This is a possible fix for issue #10684

Description

Currently in docs, the tapping on SpeedDial button will trigger onMouseEnter and onFocus in mobile before calling onClick, i.e. the open state transitions from

false (initial state) > true (onMouseEnter) > true (onFocus) > false (onClick toggle)

Solution

Need to know whether page is opened in touch screen. It can be checked through

if (typeof document !== 'undefined') {
   isTouch = 'ontouchstart' in document.documentElement;
}

and set the SpeedDial props

<SpeedDial
  onFocus={isTouch ? undefined : this.handleOpen}
  onMouseEnter={isTouch ? undefined : this.handleOpen}
>

Really appreciate for a review!

@oliviertassinari oliviertassinari added the component: speed dial This is the name of the generic UI component, not the React module! label Mar 19, 2018
Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

@AlbertLucianto Thanks for looking into it. The fix looks good to me, but I will let @oliviertassinari have the final say.

@oliviertassinari
Copy link
Member

@mbrookes I don't want to focus on extra components before the release of stable v1. I trust you with this. I won't review it;

@mbrookes mbrookes merged commit 127d275 into mui:v1-beta Mar 19, 2018
@mbrookes
Copy link
Member

@AlbertLucianto Thanks!

@mbrookes
Copy link
Member

@AlbertLucianto Unfortunately this may be problematic for hybrid touch / mouse devices (Surface Pro etc.) Could you take another look? I won't revert this just yet.

@AlbertLucianto
Copy link
Contributor Author

@mbrookes You're correct, I haven't considered such cases. Let me try work a little bit more about that. If you have any suggestion, I'd be very happy to know!

@mbrookes
Copy link
Member

@AlbertLucianto neither had I - @oliviertassinari pointed it out. (Just wish it was before I merged this 😄). It's a tricky one for sure.

@AlbertLucianto
Copy link
Contributor Author

@mbrookes Actually, one solution could be by adding onTouchStart={this.handleTouch} and

handleTouch = e => {
  e.stopPropagation();
  this.handleClick();
};

So that the default mouse-emulation handling doesn’t occur. However, it still does not work correctly.

One cause may be related to this issue in React about onTouchStart's preventDefault which is still open.

@MathiasKandelborg
Copy link
Contributor

Sorry for the long silence, I didn't notice the PR being referenced to my issue.

I'll be sure to verify and test as soon as I get some spare time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: speed dial 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.

4 participants