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

[AppBar] Hide and Elevate on Scroll #15522

Merged
merged 32 commits into from
May 4, 2019
Merged

[AppBar] Hide and Elevate on Scroll #15522

merged 32 commits into from
May 4, 2019

Conversation

cvanem
Copy link
Contributor

@cvanem cvanem commented Apr 30, 2019

Closes #12337

May-02-2019 13-06-13

This PR does the following:

  1. Add PaperProps to AppBar. This allows the underlying Paper className or classes to be overridden (for the elevate transitions). It turns out this was not actually needed.
  2. Add useScrollTrigger hook. Scroll logic required for triggering hide and elevate transitions.
  3. Add demos for hiding and elevating the AppBar on scroll.
  4. Updates DemoFrame to pass a window reference to it's children. I needed this to bind the scroll events for the demos.

Please review as this is my first PR of substance.

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 30, 2019

Details of bundle changes.

Comparing: 1ee592a...76171b8

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 0.00% 310,611 310,657 84,595 84,596
@material-ui/core/Paper +0.07% 🔺 +0.07% 🔺 67,705 67,751 20,130 20,144
@material-ui/core/Paper.esm +0.08% 🔺 +0.07% 🔺 61,003 61,049 19,038 19,052
@material-ui/core/Popper 0.00% 0.00% 28,590 28,590 10,290 10,290
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,381 2,381
@material-ui/core/TrapFocus 0.00% 0.00% 3,780 3,780 1,577 1,577
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,943 15,943 5,777 5,777
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 977 977
@material-ui/lab 0.00% 0.00% 139,644 139,644 42,535 42,535
@material-ui/styles 0.00% 0.00% 51,234 51,234 15,156 15,156
@material-ui/system 0.00% 0.00% 11,765 11,765 3,924 3,924
Button 0.00% 0.00% 85,318 85,318 25,686 25,686
Modal 0.00% 0.00% 20,366 20,366 6,692 6,692
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,531 51,531 11,368 11,368
docs.main +0.01% 🔺 +0.01% 🔺 648,808 648,883 202,575 202,593
packages/material-ui/build/umd/material-ui.production.min.js +0.02% 🔺 +0.01% 🔺 292,314 292,360 82,476 82,482

Generated by 🚫 dangerJS against 76171b8

@oliviertassinari oliviertassinari added component: app bar This is the name of the generic UI component, not the React module! new feature New feature or request labels Apr 30, 2019
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.

This looks great!

docs/src/pages/demos/app-bar/app-bar.md Outdated Show resolved Hide resolved
docs/src/pages/demos/app-bar/app-bar.md Outdated Show resolved Hide resolved
docs/src/pages/demos/app-bar/app-bar.md Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari self-assigned this May 1, 2019
@oliviertassinari oliviertassinari removed their assignment May 1, 2019
@oliviertassinari
Copy link
Member

@cvanem I have done the following changes:

  • Document the useScrollTrigger() API.
  • Move the ref responsibility to the parent, like with do for the Popper as opposed to the previous API.
  • Allow a getTrigger option but hide it from the TypeScript definitions and the documentation.

@adelin-b
Copy link

adelin-b commented May 2, 2019

How will it integrate with https://material-ui.com/demos/app-bar/#bottom-app-bar can we choose the direction of the scrolling and the behavior associated ?
I've seen implementation where both Top AppBar and Bottom AppBar hide on scroll down, but also where bottom bar hide on scroll up. So choosing the scroll direction and the hide direction seem good enough to cover the cases

@oliviertassinari
Copy link
Member

@adberard We could add a direction option For this use case as well as the horizontal scroll one.

@cvanem
Copy link
Contributor Author

cvanem commented May 2, 2019

@oliviertassinari This looks excellent. You make it look easy. Do the tests look adequate? We may want to test scrollTop instead of pageYOffset when using mountWrapperWithRef. It shouldn't make a difference but that would be more representative of real world usage I believe.

@oliviertassinari
Copy link
Member

@cvanem I have barely updated the tests. If you feel they need more refinement, why not.

@oliviertassinari oliviertassinari self-assigned this May 3, 2019
@oliviertassinari oliviertassinari removed their assignment May 3, 2019
@oliviertassinari oliviertassinari merged commit 9b1bb8a into mui:next May 4, 2019
@oliviertassinari
Copy link
Member

@cvanem Great cooperation, thank you for pushing it forward!

@verekia
Copy link

verekia commented May 5, 2019

That's fantastic! Good job :)

@ericvera
Copy link

ericvera commented May 5, 2019

This is great to see! Any ideas on which version this will ship and timeframe?

@oliviertassinari
Copy link
Member

@ericvera This change will be released in v4.0.0-beta.1, today.

@eps1lon eps1lon mentioned this pull request May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: app bar This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AppBar] Hide on scroll
8 participants