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

Improve dom_id uniqueness in guides #50988

Merged

Conversation

p8
Copy link
Member

@p8 p8 commented Feb 6, 2024

All headers in a guide get a unique dom_id to make anchor links work.
If a header is already present we would prefix it (and it's duplicate) with the dom_id of the parent node.
This would raise an error for headers without parent nodes:

../rails/guides/rails_guides/markdown.rb:47:in `dom_id': undefined method `[]' for nil:NilClass (NoMethodError)

          dom_id = "#{nodes[-2][:id]}-#{dom_id}"

This commit simplifies the dom_id uniqueness by only prepending the
parent node if it exists. This can still result in duplicates at the
same level, but for these we already show a warning:

*** DUPLICATE ID: "some_id", please make sure that there are no headings with the same name at the same level.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the docs label Feb 6, 2024
@p8 p8 force-pushed the guides/improve-dom-id-uniqueness branch from 80b96e5 to 1934554 Compare February 6, 2024 20:04
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Does it make it more difficult to target those anchors in internal links within the guides (in case we want to point to them manually for instance -- not common but possible), because they'll get the node index now?

I mean, previously we could link by hand knowing they are prefixed by parent node, now it's index base so not as easy to do it.

@p8
Copy link
Member Author

p8 commented Feb 7, 2024

Yes, I'll change it a bit. We can leave the parent node dom_id as those should be uniq anyway.

@p8 p8 force-pushed the guides/improve-dom-id-uniqueness branch 4 times, most recently from 99b9248 to e59bc3c Compare February 8, 2024 06:43
@p8
Copy link
Member Author

p8 commented Feb 8, 2024

@carlosantoniodasilva I've made the changes.
For top level headings it just uses the dom id of the heading.

Copy link
Member

@carlosantoniodasilva carlosantoniodasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and works great, thanks for the fix! Just had one minor suggestion for consideration, not a blocker.

(test failure is unrelated)

guides/rails_guides/markdown.rb Outdated Show resolved Hide resolved
All headers in a guide get a unique `dom_id` to make anchor links work.
If a header is already present we would prefix it with the `dom_id` of
the parent node. This would not work for headers without parent nodes.

This commit simplifies the `dom_id` uniqueness by only prepending the
parent node if it exists. This can still result in duplicates at the
same level, but for these we already show a warning:

    *** DUPLICATE ID: 'some_id', please make sure that there are no
    headings with the same name at the same level.

Co-authored-by: Carlos Antonio da Silva <carlosantoniodasilva@gmail.com>
@p8 p8 force-pushed the guides/improve-dom-id-uniqueness branch from 955a275 to a720480 Compare February 8, 2024 18:19
@p8
Copy link
Member Author

p8 commented Feb 8, 2024

Thanks @carlosantoniodasilva I've made the change.

@carlosantoniodasilva carlosantoniodasilva merged commit 1007cc8 into rails:main Feb 8, 2024
4 checks passed
@carlosantoniodasilva
Copy link
Member

Perfect, thanks!

@p8 p8 deleted the guides/improve-dom-id-uniqueness branch February 8, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants