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: account for headerHeight when restoring state #1110

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

phazei
Copy link
Contributor

@phazei phazei commented Jul 14, 2024

RE: #1081

@petyosi
Copy link
Owner

petyosi commented Jul 16, 2024

Thanks, I will test and merge if no issues.

@petyosi petyosi force-pushed the fix-restoreState-with-header-height branch from fbf226c to 6e37255 Compare July 20, 2024 03:58
@petyosi petyosi merged commit 370023b into petyosi:master Jul 20, 2024
Copy link

🎉 This PR is included in version 4.7.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

@phazei
Copy link
Contributor Author

phazei commented Oct 8, 2024

So, it looks like my original change was modified and moved into the stateLoadSystem.ts where it does the scrollTop -= headerHeight.

The issue with that is headerHeight isn't loaded at that point yet, it's value is always 0, so it doesn't seem to fix the issue.

In the original PR it was in the initialTopMostItemIndexSystem.ts.

        skipFrames(4, () => {
          u.handleNext(scrollTop, () => {
            u.publish(scrolledToInitialItem, true)
          })

          //by this frame the values have loaded if they exist
          const currentHeaderHeight = u.getValue(headerHeight)
          if (typeof initialTopMostItemIndex === 'object' && typeof initialTopMostItemIndex.offset === 'number') {
            initialTopMostItemIndex.offset -= currentHeaderHeight
          }

          u.publish(scrollToIndex, initialTopMostItemIndex)
        })

Those skipFrames is very important. I did test different skipFrames, and unless it's at least 3, the value of headerHeight isn't yet available. I don't know where or when it's calculated, but it needs to be sooner if it's going to be inside stateLoadSystem, because header isn't available as it is now.

@petyosi
Copy link
Owner

petyosi commented Oct 8, 2024

@phazei - the commit I pushed includes an example. As far as I remember, it works as expected. Can you elaborate? Perhaps a sandbox would help me understand the problem better.

@phazei
Copy link
Contributor Author

phazei commented Oct 8, 2024

I'm currently investigating, things might be pointing to something changing in our codebase, I haven't figured out why it's ignoring the height currently. Will update when I do!

@phazei
Copy link
Contributor Author

phazei commented Oct 22, 2024

Seems to have gone away, phantom issue 🤷 working fine now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants