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

Trigger Re-Render when Scrollability Changes and PermanentTrack is Enabled #172

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

davidgolden
Copy link

Description

There is an edge case that exists where if permanentTrackY/X is enabled and the scrollability of the child content changes, a re-render is not triggered and therefore the component remains in its previous scrollable state. This happens because the primary place where a re-render is triggered in the update method is from updating the state only when scrollbar visibility has changed. This obviously never happens when permanentTrackX/Y is enabled. This PR adds a check for if the scrollability has changed and forced a re-render if it has.

Use case

This addresses issue #171 and can be replicated in https://codesandbox.io/s/sharp-engelbart-fnc2e?file=/src/App.js. It is applicable in situations when the scrollability of the underlying content changes, such as with dynamic content.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • [X ] Perform a code self-review
  • [X ] Comment the code, particularly in hard-to-understand areas
  • Update/add the documentation - since this is a bugfix I don't think this is necessary
  • Write tests
  • Ensure the test suite passes (yarn test) - tests were already not passing. this did not cause any additional tests to fail.
  • Provide 100% tests coverage - see above

… if it has force a re-render. This handles the condition of permanentTracks enabled because otherwise we're depending on trackVisibility changing to trigger a re-render.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants