-
-
Notifications
You must be signed in to change notification settings - Fork 564
fix(vitepress-twoslash): better handling of offscreen twoslash poppers #1117
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
base: main
Are you sure you want to change the base?
fix(vitepress-twoslash): better handling of offscreen twoslash poppers #1117
Conversation
✅ Deploy Preview for shiki-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for shiki-matsu ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1117 +/- ##
=======================================
Coverage 88.35% 88.35%
=======================================
Files 74 74
Lines 3322 3322
Branches 956 956
=======================================
Hits 2935 2935
Misses 344 344
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| args: any, | ||
| ) => void | ||
|
|
||
| export function patchFloatingVueMethods(app: App, methodOverrides: FloatingVueMethodsOverrides): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little worried about the patch. Do you think to contribute upstream to fix the problem or expose the API directly first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, patching it didn't feel ideal but I still went for it since it was already done in #1090!
I'll definitely take a shot upstream on either floating-vue or floating-ui.
Should I close this PR or just convert to draft?
Also, I think your point also applies to #1116, both issues may be fixed by exposing the relevant API upstream!
And I've updated the repro link which was outdated
Description
vitepress-twoslashmakes the assumption that we're both:--vp-z-index-local-navcustom property).VPLocalNavcomponent.In other words, using twoslash ALWAYS renders the poppers in the top-left corner . In most VitePress-powered websites, relying on the
--vp-z-index-local-navcustom property would indeed luckily hide offscreen poppers becauseVPLocalNavis higher in the stacking context (poppers are simply hidden in the background).But the whole thing breaks whenever the layout doesn't follow those assumptions, e.g. either:
--vp-z-index-local-navcustom propertyVPLocalNavHere's a straightforward repro of the issue, even when using the default theme
My two cents
Code-blocks from inactive code-groups (or even better hidden blocks) should not be displayed. They may be rendered for performance purposes, but showing them obviously is an undesirable behaviour (cf the above screen).
The current approach is both smart and naive. It's not versatile enough since it relies on a specific behaviour of a specific theme. THose are prerequisites that, as I mentioned, not everyone meet.
Because there's no official API between floating-vue, VitePress and Twoslash, the easiest way of addressing this issue is to hook into floating-ui's
recomputePositionmethod to either hide or show a popper depending on whether its part of displayed/hidden block.Good news is that floating-ui's
recomputePositionis reachable throughfloating-vue, which means custom logic could be added by overriding floating-ui'srecomputePosition method. Even better news is thatvitepress-twoslashalready patches floating-vue to address some other issues.What this PR does
Poppperwrapper that makes it easieer to customizefloating-vue.