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

FIX Don't change scroll position when loading #171

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

GuySartorelli
Copy link
Member

The issue was caused by not rendering all of the elements that make up the list of links in the field while loading - which meant they're removed from the DOM, which adjusts the total height of the window.
Once the items are added back to the DOM, the window height adjusts again and therefore the user is not at the same scroll point they were at before the load event occurred.

This is fixed by following the pattern dnadesign/silverstripe-elemental uses - we render the loading component on top of the existing list of items.

Screenshot of some link fields and an elemental area, all loading

The buttons in the link fields can still be accessed by tabbing to them, so I've disabled them when we're loading.

Issue

Comment on lines +74 to +76
// Short wait - we can't use screen.find* because we're waiting for something to be removed, not added to the DOM
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 100));
});
Copy link
Member Author

@GuySartorelli GuySartorelli Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The act() here and in other tests is needed to avoid a warning:

Warning: An update to LinkField inside a test was not wrapped in act(...).

When testing, code that causes React state updates should be wrapped into act(...):

  act(() => {
    /* fire events that update state */
  });
  /* assert on the output */

This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test locally, works good, could of minor import changes

client/src/components/LinkPicker/tests/LinkPicker-test.js Outdated Show resolved Hide resolved
client/src/components/LinkField/tests/LinkField-test.js Outdated Show resolved Hide resolved
@emteknetnz emteknetnz merged commit f7fd63a into silverstripe:4 Jan 15, 2024
10 checks passed
@emteknetnz emteknetnz deleted the pulls/4/fix-loading branch January 15, 2024 20:28
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