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

UseScrollTrigger does not trigger on-page-reload in gatsby production page #20676

Closed
ohlr opened this issue Apr 21, 2020 · 14 comments · Fixed by #20678 or #20680
Closed

UseScrollTrigger does not trigger on-page-reload in gatsby production page #20676

ohlr opened this issue Apr 21, 2020 · 14 comments · Fixed by #20678 or #20680
Labels
bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. hook: useScrollTrigger

Comments

@ohlr
Copy link
Contributor

ohlr commented Apr 21, 2020

Using gatsby witth gatsby-theme-material-ui the useScrollTrigger does not trigger on page reload when the page is build.
In dev mode the behavior differs as one reloads on the top of the page not in the middle.

const trigger = useScrollTrigger({ disableHysteresis: true, threshold: 50 });

When I am in the middle of the page and reload, the trigger should fire but it does not.

@material-ui/core": "^4.9.0
gatsby-theme-material-ui": "^1.0.8
"gatsby": "^2.15.24",

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 21, 2020

@ohlr Do you have a reproduction we can look at?

@ohlr
Copy link
Contributor Author

ohlr commented Apr 21, 2020

@oliviertassinari oliviertassinari removed the status: waiting for author Issue with insufficient information label Apr 21, 2020
@oliviertassinari
Copy link
Member

@ohlr, awesome, thanks. I can reproduce the issue.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Apr 21, 2020
@oliviertassinari
Copy link
Member

@ohlr What do you think of this diff? The trick is to pull out values from the target as soon as available, instead of waiting for the next scroll event. It helps with Gatsby, but also help with Next.js in dev mode during live updates.

diff --git a/packages/material-ui/src/useScrollTrigger/useScrollTrigger.js b/packages/material-ui/src/useScrollTrigger/useScrollTrigger.js
index 79e0e331c..07db50ff1 100644
--- a/packages/material-ui/src/useScrollTrigger/useScrollTrigger.js
+++ b/packages/material-ui/src/useScrollTrigger/useScrollTrigger.js
@@ -4,10 +4,13 @@ function getScrollY(ref) {
   return ref.pageYOffset !== undefined ? ref.pageYOffset : ref.scrollTop;
 }

-function defaultTrigger(event, store, options) {
-  const { disableHysteresis = false, threshold = 100 } = options;
+function defaultTrigger(store, options) {
+  const { disableHysteresis = false, threshold = 100, target } = options;
   const previous = store.current;
-  store.current = event ? getScrollY(event.currentTarget) : previous;
+
+  if (target) {
+    store.current = getScrollY(target);
+  }

   if (!disableHysteresis && previous !== undefined) {
     if (store.current < previous) {
@@ -23,14 +26,14 @@ const defaultTarget = typeof window !== 'undefined' ? window : null;
 export default function useScrollTrigger(options = {}) {
   const { getTrigger = defaultTrigger, target = defaultTarget, ...other } = options;
   const store = React.useRef();
-  const [trigger, setTrigger] = React.useState(() => getTrigger(null, store, other));
+  const [trigger, setTrigger] = React.useState(() => getTrigger(store, other));

   React.useEffect(() => {
-    const handleScroll = (event) => {
-      setTrigger(getTrigger(event, store, other));
+    const handleScroll = () => {
+      setTrigger(getTrigger(store, { target, ...other}));
     };

-    handleScroll(null); // Re-evaluate trigger when dependencies change
+    handleScroll(); // Re-evaluate trigger when dependencies change
     target.addEventListener('scroll', handleScroll);
     return () => {
       target.removeEventListener('scroll', handleScroll);

Do you want to submit a pull request? :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Apr 21, 2020
@marcosvega91
Copy link
Contributor

I can do It if @ohlr don't have time :D

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 21, 2020

@marcosvega91 The challenge will be with the new test case then 😉

@marcosvega91
Copy link
Contributor

@oliviertassinari yes i know i was thinking about that

@oliviertassinari
Copy link
Member

@marcosvega91 In this case, we could have two pull requests, one for the fix, and one for the test, how does it sound?

@marcosvega91
Copy link
Contributor

@oliviertassinari no problem :), i'll wait for @ohlr maybe he want's to write the test case :)

@ohlr
Copy link
Contributor Author

ohlr commented Apr 21, 2020

Feel free to do it @marcosvega91.
@oliviertassinari this project is truly amazing - I am a big fan! Thanks for your work.

@oliviertassinari
Copy link
Member

@ohlr Last year, around this period, we have started to prioritize work on interoperability with Gatsby and more broadly, for people building static websites with React. While our effort starts to play-off: https://npm-stat.com/charts.html?package=gatsby-plugin-theme-ui&package=gatsby-plugin-material-ui, I feel we could still do better. The main chunk we have plan for this story is to provide a new theme outside of the box (in addition to the default Material Design one we have currently). Is there anything else you would like to see coming? New components, e.g. Scrollspy, etc ?

@ohlr
Copy link
Contributor Author

ohlr commented Apr 21, 2020

@oliviertassinari I think since static pages are more broadly used in commercial sites (where SEO matters) more corporate templates would help to gain speed quickly (a really good example which is not in the store https://github.com/dunky11/react-saas-template) all the paid ones are Admin templates.

The documentation regarding server-side rendering (Gatsby in particular) could be more specific I think. It took me quite a while to figure out that I had to use a plug-in to get rid of my production rendering-issues.
The following part could have a more detailed description: https://material-ui.com/guides/server-rendering/#reference-implementations

If I search gatsby in the docs, this is the first link that pops-up: https://material-ui.com/styles/advanced/#gatsby (quite missleading as it says one plugin but there are two (not for styles of course))

Also I was clueless about the material-ui-theme already implemented Gatsby-Link, that would have saved some time.

@oliviertassinari
Copy link
Member

@ohlr Ok, so I see two topics:

  1. more/better templates. Agree, at this point, we might need to hire a designer/developer to do us a good looking template. For instance, do you find anyone that you like in https://jamtemplates.com/ that we could bring to Material-UI? Regarding https://github.com/dunky11/react-saas-template, I'm not sure. It doesn't feel very professional. For instance, I see too many shadows. We might be able to leverage some of https://mdbootstrap.com/freebies/ & https://mdbootstrap.com/templates/.
  2. improve documentation. I'm not sure we can do more at this point 🤔

@ohlr
Copy link
Contributor Author

ohlr commented Apr 23, 2020

@oliviertassinari regarding 1) I am not an expert in template design, so maybe open a new discussion in a separate issue? And do a poll (thumbs up / down) or something :)

regarding 2) Fair point ;) maybe its just me being to lazy reading existing docs carefully ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. hook: useScrollTrigger
Projects
None yet
3 participants