-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: prevent Twoslash poppers from persisting between slides #2292
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?
Conversation
✅ Deploy Preview for slidev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@slidev/client
create-slidev
create-slidev-theme
@slidev/parser
@slidev/cli
@slidev/types
commit: |
|
The changes make a test fail. I think I should not fix this issue by using v-if, as it will destroy the components when navigation happens. |
aae15c7 to
f6bd3e8
Compare
|
The fixed popers displayed by However, It isn't worthwhile to disrupt the current rendering or preload flow just to fix this. Is there a better solution under these constraints? My thoughts might be a bit scattered — want to hear what the community and maintainers think. |
| preloadRoute(nextRoute.value) | ||
| setTimeout(() => { | ||
| hideAllPoppers() | ||
| }, 150) |
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.
This timeout time is too arbitrary. Could we find a better way to do that instead of relying on timeout?\
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.
It stems from FloatingVue's behavior. when v-popper directives are rendered to the DOM, FloatingVue immediately creates popper instances and shows them based on the :shown='true'... so I think maybe we could implement a custom Shiki transformer to intercept the Twoslash rendering process, and try to transform the code
f6bd3e8 to
e381c42
Compare
|
Seems like not the cleanest solution, but I think it works beautifully now! record.mov |
| * Custom Twoslash transformer that replaces :shown='true' with conditional logic | ||
| * to prevent poppers from being shown during preload | ||
| */ | ||
| export function transformerTwoslashConditional(): ShikiTransformer { |
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.
We should use the build-time flag rather than a custom codemod (which is unstable), like:
slidev/packages/slidev/node/options.ts
Lines 119 to 130 in 6750606
| function getDefine(options: Omit<ResolvedSlidevOptions, 'utils'>): Record<string, string> { | |
| const matchMode = (mode: string | boolean) => mode === true || mode === options.mode | |
| return objectMap( | |
| { | |
| __DEV__: options.mode === 'dev', | |
| __SLIDEV_CLIENT_ROOT__: toAtFS(options.clientRoot), | |
| __SLIDEV_HASH_ROUTE__: options.data.config.routerMode === 'hash', | |
| __SLIDEV_FEATURE_DRAWINGS__: matchMode(options.data.config.drawings.enabled), | |
| __SLIDEV_FEATURE_EDITOR__: options.mode === 'dev' && options.data.config.editor !== false, | |
| __SLIDEV_FEATURE_DRAWINGS_PERSIST__: !!options.data.config.drawings.persist, | |
| __SLIDEV_FEATURE_RECORD__: matchMode(options.data.config.record), | |
| __SLIDEV_FEATURE_PRESENTER__: matchMode(options.data.config.presenter), |
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.
Thanks so much for your review!
summarize my thoughts and questions:
- I don’t think it needs to be a configurable option. If it is not set to false, these popups will appear on pages where they don’t belong.
- Walking the Shiki-generated HAST tree and then modifying the tree (as in my latest commits). This seems better than doing string replacements — do you think this approach is stable enough?
- I’m not sure I got your point about the "build-time flag" - could you please clarify what this flag is supposed to do in your mind?
2c95811 to
cc40ca8
Compare
Prevent Twoslash poppers from persisting when navigating across slides by watching
currentSlideRoute.Fixes #2202