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

fix(table-of-contents): ensure anchor is visible upon jump-to-content #4297

Merged

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented Oct 26, 2020

Related Ticket(s)

Refs #4264.

Description

Introduces bx--tableofcontents__contents CSS class that ensures <a name="anchorname"> is visible upon getting scroll to by selection of an item in <dds-table-of-contents>.

Changelog

New

  • bx--tableofcontents__contents CSS class.

Removed

  • Code setting the margin offset of InsersectionObserver for scroll hijacking code. It's effectively replaced with the new bx--tableofcontents__contents CSS class.

Introduces `bx--tableofcontents__contents` CSS class that ensures
`<a name="anchorname">` is visible upon getting scroll to by selection
of an item in `<dds-table-of-contents>`.

Refs carbon-design-system#4264.
@asudoh asudoh added the package: web components Work necessary for the IBM.com Library web components package label Oct 26, 2020
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 26, 2020

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 26, 2020

<a name="3" data-title="Section - 3"></a>
<h3>Section - 3</h3>
<p>Section 3 content</p>
<div class="bx--tableofcontents__contents">
Copy link
Member

Choose a reason for hiding this comment

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

@asudoh are we replying on adopters to add this wrapper class? Anyway this element can be transcluded instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asudoh are we replying on adopters to add this wrapper class?

Yes. Seems that we need this kind of wrapper class for React, too, given navigating directly to hashbang URL fails there.

Anyway this element can be transcluded instead?

Do you have any more details here...? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is can we add the wrapper div for the user instead of asking them to add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for clarifying @jeffchew! I thought of making it automatic, but the problem is that our style (shadow DOM style) doesn't have direct control over the content style, and we allow arbitrary content for <dds-table-of-contents>. Therefore it looks like it's more prudent for us to clearly document that user has to use our CSS class to adjust the anchor position. I used below wording as an explanation of what the example code does, but good to hear better wording, etc.:

💡 bx--tableofcontents__contents class in the content <div> ensures the <h3> is visible by navigating to a hashbang URL (e.g. https://yoursite.com/yourpage#2).

Copy link
Member

@kennylam kennylam left a comment

Choose a reason for hiding this comment

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

Looks good thanks @asudoh!

@kennylam kennylam added the Ready to merge Label for the pull requests that are ready to merge label Nov 2, 2020
@kodiakhq kodiakhq bot merged commit f6520d6 into carbon-design-system:master Nov 2, 2020
@asudoh asudoh deleted the table-of-contents-content-offset branch November 2, 2020 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants