[useScrollTrigger] Enhance trigger, improve tests

[cvanem]

• I have followed (at least) the PR section of the contributing guide.

Let me know if you want me to create separate issues for the below items. This feature is relatively new so a PR alone may be appropriate.

Hook Updates

1. Re-evaluate the trigger when dependencies change (i.e threshold prop changes externally). We are already using this logic for the initial trigger value. We should also use it for any updated trigger values when dependencies change.

Default Trigger Updates

1. Correctly handle sequential hysteresis triggers where scroll.current == previous. Previously this would return false when scroll.current = previous. Example scenarios: a scroll event is triggered that only changes pageXOffset or a user manually dispatches a scroll event that doesn't change the X or Y offset.
2. As a result of the above items, the trigger needs to use the previous value when the event is null, instead of always defaulting to 0.

Test Updates

1. Fix a bug in testing logic, changed pageYoffset => pageYOffset. This was preventing some tests from evaluating.
2. Fix tests to use disableHysteresis prop instead of the old directional prop.
3. Add new tests for above scenarios.

[mui-pr-bot]

No bundle size changes comparing 1555e52...192cc8e

Generated by 🚫 dangerJS against 192cc8e

[oliviertassinari]

@cvanem Thank you for the follow-up, I have tried to simplify the logic in 48e3fc8. Let me know if it's alright.

[cvanem]

@oliviertassinari It's a little early here, but I ran through all the combinations of undefined, zero and a number for current & previous and I think it checkouts out. It also works correctly in my application use case. We can also simplify a little more and delete this line right?

return store.current >= previous && store.current > threshold;

I think it is redundant and not needed after the changes you made.

[oliviertassinari]

I think it is redundant and not needed after the changes you made.

@cvanem To try, does it break the tests?

[cvanem]

@oliviertassinari It passed when I tested locally earlier. I can look at it more tomorrow if needed or you can push it if you want. I’m AFK right now.

[oliviertassinari]

What's the exact diff that I should test?

[cvanem]

@oliviertassinari I was going to push more tests, but I was wondering if you think we need to support negative threshold values or not? Currently, I think it breaks with negative thresholds.

[oliviertassinari]

@cvanem How can the scroll value be negative in the first place?

[cvanem]

@oliviertassinari I am pretty sure I have seen negative values on mobile devices with scroll bouncing and over scroll. I think it can also happen on desktops using touch devices for scrolling, like here. I don't have a desktop touch device to test easily. According to this issue, it can happen on MacOS.

[cvanem]

@oliviertassinari To effectively track the trigger state with 0, undefined and negative values, I think we were missing the initialState. I went ahead and added an undocumented initialState prop and added more exhaustive tests. It seems to pass all the tests I could throw at it. Let me know what you think or if we should go back to the previous implementation?

[oliviertassinari]

@cvanem I don't think that we need an initialState, the trigger should be determined by the previous and current state. I have removed this option. The logic should be able to handle negative values. I have simplified the tests. I have removed the brute force tests and skipped the non-JSDOM environments. Thank you for your time!

[oliviertassinari]

The threshold is exclusive now, it was inclusive before. I don't know what's the best option, I have updated the documentation at least.

[cvanem]

@oliviertassinari Ok thanks for the update. Just curious, were the brute force tests removed because it is bad practice or for speed?

[oliviertassinari]

@cvanem It was slow, but more importantly, it was testing all the combinations of a few branch logic. I think that we are better off with the scenarios you first wrote. They are closer to how the hook is used in real life.