mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.74k stars 32.24k forks source link

[AppBar] Hide on scroll #12337

Closed verekia closed 5 years ago

verekia commented 6 years ago

It is a common UX pattern to hide the AppBar when scrolling down on small screens.

There are different levels of implementation that are possible:

I implemented it up to the Sliding part as a HOC to apply to AppBar but it's not as nice as having it natively supported by Material UI (demo of my implementation). Some Android examples.

The snapping feature is apparently a thing that got added in Android Library 23.1 (section Design).

oliviertassinari commented 6 years ago

@verekia Thanks for sharing your work! This scrolling behavior is documented in the Official specification. It's something specific to small screens. I like the API you have been using, using a higher order component looks like a good tradeoff. Looking at the implementation, it's not something I think that we should copy paste as it is. For instance, we should be able to remove recompose and lodash dependency.

I'm not sure if it's possible to even do that second line of items with Material UI, but it's worth noting here.

Yes, you can. It's just we don't provide any specific API for it yet.

verekia commented 6 years ago

Good call about the official doc, although they don't give much details. Yes, my HOC is really just a hopefully temporary crutch. It can be useful to give some basic guidance, but it's definitely not to take as-is.

cvanem commented 5 years ago

@verekia @oliviertassinari Do we still want to support this feature? What about something like this? https://codesandbox.io/s/7wok93mo20

I think this would provide the means to support all of the scrolling behavior outlined in the specification and it would be fairly easy to add to the AppBar component if we add a separate CollapseOnScroll hook. I do wonder if this is the best way to structure the functionality and if there are any performance concerns with handling the onScroll event? Some other libraries do throttling on the event handling.

chr-knafl commented 5 years ago

Hello!

I just started building my app with react and material ui and was searching for that feature, as I do believe is very important for the user experience. Vuetify supports this feature with a property on the component called "scroll-toolbar-off-screen" (see https://vuetifyjs.com/en/components/toolbars). Besides that there are many more properties for configuring this behaviour (e.g. scroll-threshold, ...)

I really appreciate your efforts and wait for that feature to be included.

BR Christoph

cvanem commented 5 years ago

@chr-knafl I think the Material team must busy with the 4.0 release. Once that is done, I'm hoping we can look at getting this feature added. I am willing to work on a PR for this feature, but I think I would need a little guidance to be successful.

chr-knafl commented 5 years ago

@cvanem Thanks for your answer! Your solution you added in the codesandbox seems to work properly. I did not test it on other browser than my chrome in the last version though. If there are some issues I will let you know! BR Christoph

oliviertassinari commented 5 years ago

I agree that it's a useful feature and that a hook would be an excellent fit for the problem.

@cvanem Regarding the performance, you have an issue here. We need to reduce the number of re-rendering to the number of time the public API changes. Once we have a working hook, we might want to add a in like prop to the AppBar component to animate between the exit and enter state. Collapse doesn't provide the same animation. Maybe Slide

@chr-knafl Do you have a live example on Vuetify side? I can't see it working in your link.

Here is how I would approach the problem with v4, it's not perfect but a great start:

import React from "react";
import { Slide } from "@material-ui/core";

function getScrollY(scroller) {
  return scroller.pageYOffset !== undefined
    ? scroller.pageYOffset
    : scroller.scrollTop !== undefined
    ? scroller.scrollTop
    : (document.documentElement || document.body.parentNode || document.body)
        .scrollTop;
}

const useHideOnScroll = options => {
  const { threshold, scroller } = options;

  const scrollRef = React.useRef();
  const [hide, setHide] = React.useState(false);

  const handleScroll = React.useCallback(() => {
    const scrollY = getScrollY(scroller || window);
    const prevScrollY = scrollRef.current;
    scrollRef.current = scrollY;

    setHide(
      scrollY < prevScrollY
        ? false
        : scrollY > prevScrollY &&
          scrollY > (threshold != null ? threshold : 100)
        ? true
        : false
    );
  }, [scroller, threshold]);

  React.useEffect(() => {
    window.addEventListener("scroll", handleScroll);
    return () => {
      window.removeEventListener("scroll", handleScroll);
    };
  }, [handleScroll]);

  return hide;
};

export default function HideOnScroll(props) {
  const { children, threshold, scroller, ...other } = props;

  const hide = useHideOnScroll({ threshold, scroller });

  return (
    <Slide direction="down" appear={false} in={!hide} {...other}>
      {children}
    </Slide>
  );
}

https://codesandbox.io/s/5x8kq3nwjn (notice the addition of Box, CssBaseline and Container)

Apr-26-2019 23-07-20

cvanem commented 5 years ago

@oliviertassinari Here is my first attempt at a real implementation: https://codesandbox.io/s/kknzvyq3p3?fontsize=14

I would appreciate it if someone could take a look and let me know if I am on the right track or should be headed in a different direction. Also, not sure if all of these changes would be allowed? I imagine they should be discussed before submitting a PR.

1. AppBar.js - I added the following props:

I was thinking we could possibly add a new Elevate transition component? This could replace the elevation logic in Paper.js and also be used here by passing in via TransitionComponent. If this is done, the new in and variant props would not be needed and can be removed. Just a thought, maybe there is a better way to accomplish this?

2. App Bar Expansion in Specification

I believe this implementation covers all of the App Bar scrolling behaviors outlined in the Official Specification except the expand example. I played around a bit with using the AppBar + ExpansionPanel. This approach works, but I am not sure if it is the correct approach as you are then limited to the expansion panel structure. Maybe the expansion implementation is for a later date...

3. demo.js - On Scroll logic and related hooks:

The hooks and scroll logic are pretty close to being finished. I think that the even't handlers may still need to be throttled as I am seeing some weird behavior sometimes on the Collapse with momentum scrolling. I ended up writing one hook for useTriggerOnScroll, which takes a triggerFunc parameter. This way, it can be used for both the elevation and hide triggers, which have a little different logic (see demo.js). Is this functionality something that could be added to the core package somewhere as a utility or should this be implemented by the user?

4. Example documentation

I added the above code as a separate example to the documentation, which appears to demonstrate the AppBar fairly well. It is pretty hard to show the user all the different settings, especially with static and fixed positions, and have it look good as a demo. Is there a better way to do the example documentation? Most AppBar's are typically set to 'fixed', but this doesn't show all that well in the example section. Maybe we should limit the user to only using pre-defined settings, or expose the pre-defined settings through different AppBar variants?

oliviertassinari commented 5 years ago
  1. AppBar.js - I added the following props:

@cvanem I think that we should avoid having the AppBar importing any transition component, nice call by not doing it. However, It seems that we only need the missing animation for the elevation. It should probably be added to the Paper component.

  1. App Bar Expansion in Specification I believe this implementation covers all of the App Bar scrolling behaviors outlined in the Official Specification except the expand example.

Do we need to handle it? It looks too complex and has a niche use case to be considered now. I would say no.

  1. demo.js - On Scroll logic and related hooks:

We shouldn't need a throttle function in the first place—I haven't looked at it in detail—it's the sign something else is off. I haven't considered the elevation case. Nice, here is an updated proposal: https://codesandbox.io/s/qx186qjqr9. I would probably only make the useAppBarScroll() hook public while adding demos for the Elevation component and HideOnScroll component. What do you think about it?

  1. Example documentation

We have an iframe option to render the demos in an iframe. It would be appropriate in our case. I like the interactive demo idea. We only need to be careful, we shouldn't provide too many options people will be confused about.

cvanem commented 5 years ago

@oliviertassinari

Thanks for the feedback. I like your simplified approach much better.

For the Scroll hook logic, a few things:

  1. I prefer a more generic hook that can be customized and open to more use cases. After reading, take a look at my new approach in useScrollTrigger.js. It uses the default trigger behavior from useAppBarScroll, but also allows customization. I think it is better as it addresses items a, b and c below, but I am fine if you want to use a more direct useAppBarScroll approach. Just curious, are there any other material-ui components that could benefit from a shared scroll trigger hook? I am considering publishing a react-use-scroll-trigger library as I haven't been able to find an implementation that does everything I want, which is:

    • a) Minimize hook update frequency and return a flag, aka trigger, only when a condition is met.
    • b) Implement a default trigger condition and also allow it to be customized.
    • c) Default the scroll target to window and also allow a ref to be passed in while correctly handling the ref state. More info here.
  2. Do we want the ability to specify a scroll target ref other than window? If so, I think we will need to provide a setRef callback instead of simply passing in the ref. Same reason/link as item c above

Here is my new approach, with your file structure: https://codesandbox.io/embed/7zjk7n51o0

See the changes in these files:

chr-knafl commented 5 years ago

Do you have a live example on Vuetify side? I can't see it working in your link.

@oliviertassinari No, unfortunately not, I just read the official documentation of Vuetify. Maybe I can provide an example within the next weeks (I am currently quite busy - sorry).

oliviertassinari commented 5 years ago

@cvanem I love it! 😍🔥 Really nice. It's mature enough for opening a pull request. This way, we can discuss the changes in detail, line by line.

fukemy commented 3 months ago

Hi there, I am using ScrollToHide AppBar + Drawer, the problem I have got is when AppBar go hided, then I saw the space from Drawer to top, can anyone guide me to fix it