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.

Sign up for GitHub

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

Jump to bottom

[BottomNavigation] onClick does not fire if tapped while scrolling #22524

Merged
merged 15 commits into from Sep 9, 2020
Merged

[BottomNavigation] onClick does not fire if tapped while scrolling #22524

merged 15 commits into from Sep 9, 2020

Conversation

EliasJorgensen
Copy link
Contributor

@EliasJorgensen EliasJorgensen commented Sep 7, 2020 •
edited by oliviertassinari

Closes #22368.
The onTouchStart and onTouchEnd events are now also used to decide whether or not a given BottomNavigationAction is clicked.

As per @oliviertassinari's request, a demo has been added showcasing how to fix BottomNavigation to the bottom of the page.

@oliviertassinari oliviertassinari added component: bottom navigation This is the name of the generic UI component, not the React module! new feature New feature or request labels Sep 7, 2020
@github-actions github-actions bot mentioned this pull request Sep 7, 2020
Closed
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 7, 2020 •
edited

@material-ui/core: parsed: +0.15% , gzip: +0.21%

Details of bundle changes

Generated by 🚫 dangerJS against 079c34c

@EliasJorgensen
Copy link
Contributor Author

EliasJorgensen commented Sep 7, 2020 •
edited

@oliviertassinari I'm getting the Touch is not defined error a lot in the non-chrome browser tests. I can tell from MDN that TouchEvent (and therefore also Touch) is not supported in IE and Safari. Do you have an idea for how to solve this? If touch events are not supported in those browsers, i'm guessing that we are not interested in testing the touch functionality in them anyway?

@EliasJorgensen
Copy link
Contributor Author

EliasJorgensen commented Sep 7, 2020 •
edited

I guess i could make a check, short circuiting the tests to completion if Touch is not defined. It's a little bit inelegant though. I don't think i know enough about your test setup to make an educated decision.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 7, 2020 •
edited

It's what we do in https://github.com/mui-org/material-ui/blob/f37992f328fa3c2d5d08ad8296801e7ec1d29475/packages/material-ui/src/Slider/Slider.test.js#L28-L32

we could turn it into a skip(), could be cleaner.

oliviertassinari
oliviertassinari reviewed Sep 7, 2020
View reviewed changes
packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js Outdated Show resolved Hide resolved
packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js Outdated Show resolved Hide resolved
packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.js
) {
touchTimer.current = setTimeout(() => {
target.dispatchEvent(new Event('click', { bubbles: true }));
}, 10);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could have something more reliable than an arbitrary 10ms delay.

packages/material-ui/src/BottomNavigationAction/BottomNavigationAction.test.js Outdated Show resolved Hide resolved
oliviertassinari and others added 3 commits September 8, 2020 00:49
@oliviertassinari
polish documentation
6fc0f85
@EliasJorgensen @oliviertassinari
Update packages/material-ui/src/BottomNavigationAction/BottomNavigati…
ef80bcf 
…onAction.js

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@EliasJorgensen @oliviertassinari
Update packages/material-ui/src/BottomNavigationAction/BottomNavigati…
bb6b901 
…onAction.js

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@EliasJorgensen
Copy link
Contributor Author

It's what we do in

https://github.com/mui-org/material-ui/blob/f37992f328fa3c2d5d08ad8296801e7ec1d29475/packages/material-ui/src/Slider/Slider.test.js#L28-L32

we could turn it into a skip(), could be cleaner.

Agreed. I tried using a skip, but i ran into this problem: mochajs/mocha#2819
I will use a return like in some of the other tests, instead.

@EliasJorgensen
Copy link
Contributor Author

Should be all good now @oliviertassinari :)

@oliviertassinari oliviertassinari merged commit 0652c2c into mui:next Sep 9, 2020
@oliviertassinari
Copy link
Member

@EliasJorgensen It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@eps1lon eps1lon mentioned this pull request Aug 11, 2021
Open
2 tasks
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Aug 11, 2021
@eps1lon
Revert "[BottomNavigation] onClick does not fire if tapped while scro…
4b1967f 
…lling (mui#22524)"
@eps1lon eps1lon mentioned this pull request Aug 17, 2021
Merged
eps1lon added a commit that referenced this pull request Aug 17, 2021
@eps1lon
Revert "[BottomNavigation] onClick does not fire if tapped while scro…
39317f7 
…lling (#22524)" (#27690)
@eps1lon eps1lon mentioned this pull request Aug 18, 2021
Merged
@EliasJorgensen EliasJorgensen mentioned this pull request Sep 10, 2021
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
component: bottom navigation This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BottomNavigation] onClick does not fire if tapped while scrolling
3 participants
@EliasJorgensen @mui-pr-bot @oliviertassinari