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

Router: event listener on internal links is overly broad #591

Closed
3 tasks done
egardner opened this issue Mar 28, 2022 · 5 comments · Fixed by #1104
Closed
3 tasks done

Router: event listener on internal links is overly broad #591

egardner opened this issue Mar 28, 2022 · 5 comments · Fixed by #1104
Labels
enhancement New feature or request
Milestone

Comments

@egardner
Copy link

egardner commented Mar 28, 2022

Describe the bug

We are using Vitepress to power a documentation and demo site for a UI library (https://doc.wikimedia.org/codex/main/), as an alternative to Storybook.

Recently I created a new page to demonstrate a tabbed layout component, which makes use of some internal links (with @click.prevent applied) for semantic/accessibility purposes. I was surprised to see that the demo page contained some unanticipated behavior – clicking a tab caused the page to jump around and updated the URL with a fragment ID.

This is happening because the Vitepress router introduces a global event listener for all internal link elements on the page, adding some extra behavior (updating the URL hash, scrolling the target element into view). This makes sense for heading or section elements on the page, but it's being applied everywhere.

Is there any way to scope this to be a little more specific? Maybe this handler should only apply to headings or section elements, or should only target elements that have been created by Vitepress as part of the page theme?

Reproduction

Behavior can be seen here: https://772516--wikimedia-codex.netlify.app/components/tabs.html – click a tab in one of the demo components and watch the page jump around.

Expected behavior

Not expecting additional behavior to be added to links inside embedded demo components on pages.

System Info

System:
    OS: macOS 12.3
    CPU: (10) arm64 Apple M1 Pro
    Memory: 141.38 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.14.0 - /opt/homebrew/opt/node@16/bin/node
    npm: 8.3.1 - /opt/homebrew/opt/node@16/bin/npm
  Browsers:
    Chrome: 99.0.4844.83
    Firefox: 98.0.1
    Safari: 15.4

Additional context

No response

Validations

  • Follow our Code of Conduct
  • Read the docs.
  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
@egardner egardner added the bug: pending triage Maybe a bug, waiting for confirmation label Mar 28, 2022
@kiaking kiaking added enhancement New feature or request and removed bug: pending triage Maybe a bug, waiting for confirmation labels May 23, 2022
@kiaking
Copy link
Member

kiaking commented May 23, 2022

Not sure how to tackle this issue yet, but we should consider it before 1.0.0 release since this might cause breaking changes on how links work.

@kiaking kiaking added this to the v1.0.0 milestone May 27, 2022
@brc-dd
Copy link
Member

brc-dd commented May 30, 2022

@kiaking Should we introduce a new attribute data-no-click? If it is not present on an anchor or its parents, only then run that logic. This way OP can specify click events on which components should not be handled by the router's event handler.

@kiaking
Copy link
Member

kiaking commented Jun 6, 2022

Hmmm... This issue is a little related to #199, where this is about CSS but it's kinda same in a way that it would be great to have a "scoping" feature in the doc theme.

In that sense, maybe it might be better to introduce a block element or custom component for this...? Something like:

<Raw>
 <MyComponent />
</Raw>

And we ignore everything inside <Raw> component (if that's possible 🤔).

And to do that, I think adding data-no-click attribute as a first step sounds nice idea 👍

@jd-solanki
Copy link
Contributor

jd-solanki commented Jun 6, 2022

Using Raw component is definitely a nice solution but I have my input here.

Like @egardner (and me 😄), there will be cases where VitePress will get used for component demo, and adding Raw component on each page 3-4 times is not a good DX.

Sorry, I don't have a solution to share this at the moment.

@brc-dd
Copy link
Member

brc-dd commented Jun 28, 2022

So here is my solution (workaround) for CSS isolation: #199 (comment)

We probably should just check for .vp-raw instead of data-no-click. And document this😅. We can have that postcss thing built in VP too.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants