fix(ui): file tree scroll position is stable when browsing#2288
fix(ui): file tree scroll position is stable when browsing#2288jhroemer wants to merge 4 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe page component persists the file-tree sidebar’s vertical scroll per package/version. It adds a template ref Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8c1d70c-6026-4a1a-bb36-b59cca79e631
📒 Files selected for processing (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
Outdated
Show resolved
Hide resolved
ghostdevv
left a comment
There was a problem hiding this comment.
So the issue seems to be that Nuxt is re-rendering the whole page as there is a navigation that happens when you click the file? I wonder if instead we could do something like this to prevent nuxt re-rendering completely?
definePageMeta({
// ...
key: (route) => `/package-code/${route.params.org ?? ''}/${route.params.packageName}/v/${route.params.version}`,
});
Thanks @ghostdevv. Yeah I didn't really consider/question the page-reload, I kinda assumed it was intentional. I just tried it quickly and it does the trick, but there's no type inference on the route object params, and I don't know if it's possible without having to do type assertions manually or cast it? And then I'm not entirely sure about the behavior when you change versions: if you change version, maybe the collapse/expand state should be reset too, because the file-tree might change. That's the current behavior when the entire page re-renders, but not if things stay in place. Wdyt? |
So I think the code snippet I suggested (if it works, I haven't had a chance to test it) would mean that it wouldn't cause a reload if it's just the file you selected changes but would if it's the package/version. Theoretically, that would mean that Vue should be reactive and update but it's entirely possible that it won't or we're relying on the reload. I can take a look into it tomorrow? 🙏 |
🔗 Linked issue
#2286
🧭 Context
Currently browsing the code view can be somewhat annoying, since the scroll position resets on navigation.
📚 Description
It's just nice that scroll position is stable when clicking around :-)
The fix:
useState) before unmountingAs far as I can see on the Nuxt docs
flush: 'post';is good practice when you want to watch the DOM, but I rarely touch Vue code so would appreciate feedback. Same goes for theuseStatecomposable, it's the first time I'm using it 😄 But the main part is that the state should persist across navigation.Regarding testing: I would personally not get into testing scroll position. I've done this in the past which caused lots of flaky tests.
package-code-view-scroll-fix.mov
I did not find PRs that directly overlap. I did find this one but it's not the same thing + I believe it would be complimentary.