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 TableOfContents Examples links #1665

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

Kadirsaglm
Copy link
Contributor

When an example heading in the readme.md file contains markdown elements like code, the generated heading ID is based only on the text before the markdown. This can lead to duplicate heading IDs. For example, in the DateTimeInput documentation, the following examples both end up with the same ID, "A", because DateTimeInput is formatted as code:

A DateTimeInput with some disabled dates that are supplied via a string array:
https://instructure.design/#DateTimeInput/#A

A DateTimeInput with some disabled dates that are supplied via a function:
https://instructure.design/#DateTimeInput/#A

Another common issue is that when linking to these examples from the table of contents, the ID includes whitespace from before the markdown part is cut off. However, the link’s href omits the trailing whitespace, causing the link to break. For instance, in the DateTimeInput docs:

A DateTimeInput with columns layout and a default value: https://instructure.design/#DateTimeInput/#A%20DateTimeInput%20with

And corresponding header
<h4 dir="ltr" id="A DateTimeInput with " class="css-xjt99e-view-heading">A DateTimeInput with <code>columns</code> layout and a default value:</h4>

To resolve these issues, I propose assigning example headings an ID in the format example-{number-of-example}. This ensures that each heading has a unique and functional ID.

@Kadirsaglm Kadirsaglm force-pushed the fix_toc_example_links branch 2 times, most recently from bb86610 to f9c73f2 Compare September 1, 2024 18:13
@Kadirsaglm Kadirsaglm changed the title Fix ToC example links Fix TableOfContents Examples links Sep 1, 2024
@matyasf matyasf self-assigned this Sep 2, 2024
@matyasf
Copy link
Collaborator

matyasf commented Sep 2, 2024

Thanks for the PR! While I like the approach of coming up with some logic for the anchor links, I feel that this solution would introduce too many, seemingly arbitrary conventions that documentation writers would have to follow (it only works on h4 headings under a heading called Examples).

I think a better solution would be something more generic that works for all table of contents items, e.g. use `heading.innerText.toLowerCase() to remove characters that cannot be in a link.

Also while you work on this part you could look into why anchors dont work when opened as links, e.g. https://instructure.design/#Badge/#Guidelines does not scroll down to the 'Guidelines' part, just shows the top of the page.

Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

see my comment above

@Kadirsaglm
Copy link
Contributor Author

Thanks for the PR! While I like the approach of coming up with some logic for the anchor links, I feel that this solution would introduce too many, seemingly arbitrary conventions that documentation writers would have to follow (it only works on h4 headings under a heading called Examples).

I think a better solution would be something more generic that works for all table of contents items, e.g. use `heading.innerText.toLowerCase() to remove characters that cannot be in a link.

Also while you work on this part you could look into why anchors dont work when opened as links, e.g. https://instructure.design/#Badge/#Guidelines does not scroll down to the 'Guidelines' part, just shows the top of the page.

Thank you for the input. I changed the heading Id generation process in the compiledMarkdown.tsx.

I will look into the anchor issue as well.

@Kadirsaglm
Copy link
Contributor Author

The issue with scrolling was that the scrollToElement callback function was triggered before the documentation was rendered, so it couldn't find the linked headings. I moved the scrollToElement callback function to make sure it is triggered after the documentation page is rendered.

Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

I like this solution, also now anchor links work, nice job!

@matyasf matyasf merged commit 6928c97 into instructure:master Sep 12, 2024
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants