-
Notifications
You must be signed in to change notification settings - Fork 30
Add deeplink anchors to dropdown directive #1765
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
🔍 Preview links for changed docs |
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.
Thank you! 🎉
@elastic/docs-engineering PTAL, thanks! |
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.
Do we want to always ensure we can deeplink to dropdowns?
E.g infer :name:
if none is provided?
src/Elastic.Documentation.Site/Assets/open-details-with-anchor.ts
Outdated
Show resolved
Hide resolved
@Mpdreamz Used the slug class to generate names for dropdowns automatically. Had to add some extra logic to handle dropdowns with :open: because they were triggering the URL update (and we don't want that). |
…cs-builder into add-deeplink-anchors-dropdown
src/Elastic.Markdown/Myst/Directives/Dropdown/DropdownView.cshtml
Outdated
Show resolved
Hide resolved
src/Elastic.Documentation.Site/Assets/open-details-with-anchor.ts
Outdated
Show resolved
Hide resolved
src/Elastic.Documentation.Site/Assets/open-details-with-anchor.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Martijn Laarman <Mpdreamz@gmail.com>
…cs-builder into add-deeplink-anchors-dropdown
@Mpdreamz Seems like we've hundreds of duplicated dropdown names. I'm in favour of not generating anchors automatically at this point. :) https://github.com/elastic/docs-builder/actions/runs/17273537579/job/49024386232?pr=1765 |
I do think its best if this works uniformly across all drop-down, we might need to create a docs-content issue to address the duplicates on a single page which may hurt SEO too. |
@Mpdreamz So unless we turn the errors into Hints, this PR will be blocked until we make all the dropdown titles unique? |
I think we need a plan, just merging it with hints is not sufficient as hints are too easily ignored and will create PR annotations Since it effects our |
The only other option I'm thinking of is to just allow manual naming / anchoring and not generating anchors automatically. Then in a second phase we could sweep through docs sets as part of SEO improvements and add autogeneration. |
I think its best to identify all places tackle them and then merge this. The other way around creates no pressure on actually addressing the issue :) We can create a tracking issue in Will do so next week! |
Not sure how to proceed with this one. Couldn't we just remove the automatic generation of anchors? |
@Mpdreamz This now works as intended in that it automatically generates an anchor for each dropdown if one isn't set. I also added an Hint for duplicated anchors of any type (heading and dropdowns) and the assembled quantity is staggering: more than 4k duplicated anchors, if the method is correct. Since dropdown anchors do not interfere with the ToC, which is the biggest source of confusion for users, I would suggest that we move the Hint / detection logic to a separate PR and just merge this one. |
Fixes #1759
:name:
parameter into an anchor link for the dropdown title.I avoided adding a visual indicator because it would have polluted the design of the dropdown.