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

[1.x] Fix [scroll-region] on html element with overflow-scroll #1782

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

axel-paillaud
Copy link
Contributor

add restoreScrollPositions() in setPage() method if we have scroll-region. Use this.page instead of page.

This is related to #1211

In line 454 : console.log(page.scrollRegions) is always undefined, and I don't know why.
If we do only console.log(page), we can see the scrollRegions field inside browser inspector, but I think it was populated asynchronously ?

With this version, scroll-region work well, and scrolling items are not reset when we submit the form (for example), but this.page is kind of a cheat ?

Copy link
Collaborator

@pedroborges pedroborges left a comment

Choose a reason for hiding this comment

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

Tested this PR on the React and Vue3 playground following the steps to reproduce from #1211. The proposed solution fixes that issue 👍

There's another issue that prevents your solution from working in React projects but #1803 already takes care of that. I tested the code from both PRs to confirm they fully fix the issue 🙌

My only suggestion is move this.restoreScrollPositions() to outside of the else block, between if statements is fine 😉

Thanks @axel-paillaud!

@pedroborges pedroborges added the core Related to the core Inertia library label Sep 12, 2024
@pedroborges pedroborges changed the title fix scroll-region on html element with overflow-scroll [1.x] Fix [scroll-region] on html element with overflow-scroll Sep 13, 2024
Copy link
Contributor

@joetannenbaum joetannenbaum left a comment

Choose a reason for hiding this comment

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

Thank you!

I tested this out on the playground and was able to replicate both the problem and the fix. Just flagging that it feels a bit odd that page and this.page are out of sync in this way, but the fix works and I don't see any side effects, so we'll just keep an eye on it.

@axel-paillaud
Copy link
Contributor Author

You're welcome ! Glad to help.
Let me know if I should reinvestigate this PR, I admit that since it's been a while, I would have to dive back into it, but it's possible if you think it's necessary.

@pedroborges pedroborges merged commit fcc8cde into inertiajs:master Sep 19, 2024
5 checks passed
pedroborges added a commit to olragon/inertia that referenced this pull request Sep 24, 2024
* master: (95 commits)
  [1.x] Fix props reactivity (inertiajs#1969)
  [1.x] useForm wrongly overwrites default values ​​after successful submission (inertiajs#1935)
  Update changelog
  [1.x] Fix `resetScrollPositions` and `restoreScrollPositions` router methods (inertiajs#1980)
  [1.x] Fix [scroll-region] on html element with overflow-scroll (inertiajs#1782)
  [1.x] Fix useForm re-renders by memoizing functions in React (inertiajs#1607)
  [1.x] Fix "DataCloneError: <Object> could not be cloned" (inertiajs#1967)
  [1.x] preserveScroll should be true on initial page visit (inertiajs#1360)
  Fix type augmentation (inertiajs#1958)
  [1.x] Fix doubling hash in React StrictMode (inertiajs#1728)
  [1.x] Add SSR support for Svelte 5 (inertiajs#1970)
  [1.x] Fix <Render /> component to respect "preserveState" (inertiajs#1943)
  [1.x] Fix 'received an unexpected slot "default"' warning (inertiajs#1941)
  QA: Add @types/lodash to fix svelte-check
  QA: Update reactive if statement
  Review useForm types
  QA: Move the if server up
  QA: Revert tsconfig change
  QA: Remove plural
  QA: Remove unused props type + add extra types just in case
  ...

# Conflicts:
#	packages/react/src/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to the core Inertia library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants