facebook / docusaurus

Easy to maintain open source documentation websites.
https://docusaurus.io
MIT License
56.61k stars 8.5k forks source link

Page titles rendered using H1 should have an ID for permanent link #1828

Closed parkas2018 closed 4 years ago

parkas2018 commented 5 years ago

🐛 Bug Report

Currently in V1 of docusaurus, when the site is generated, it uses the title metadata of each markdown files (default/out of the box behaviour). However, this title does not have any id or anchor links unlike all other headings found in the markdown files.

This is causing a very weird behaviour when the title of the page matches search results (if algolia search is integrated). Clicking on the link from algolia search result, takes us to the correct page but the header/title of the page is tucked under the site navigation header.

I believe this is happening because the title of the page does not have an "id". When the search query matches the page title, it tries to generate a permanent link to that particular "heading". So, Algolia search result generates a permanent link using the closest parent that has an id, which is docsNav.

The issue is easier to see with an illustration.

Have you read the Contributing Guidelines on issues?

Yes

To Reproduce

  1. Go to Docusaurus' documentation site here: https://docusaurus.io/docs/en/installation
  2. In the search box, type query such as version. The search result might look like this:
    • image
  3. Click on the result that matches the title/heading of this page (the first result in the above screenshot): https://docusaurus.io/docs/en/versioning
  4. You'll be taken to a page whose URL looks like this: https://docusaurus.io/docs/en/versioning#docsNav

Expected behavior

Clicking on the search result that matches the page title should have taken me to that page anchored to the page title properly. It should be displayed like this:

image

Actual Behavior

Clicking on link (https://docusaurus.io/docs/en/versioning#docsNav) from search result takes me to the correct page but the display is skewed. The title is touching the bottom of the top navigation bar:

image

This does not happen if you try any other anchor links on the page from the right hand side table of content.

Reproducible Demo

(Paste the link to an example repo, including a siteConfig.js, and exact instructions to reproduce the issue.)

parkas2018 commented 5 years ago

@endiliey - Thanks for reviewing this bug report. Do you agree with my assessment of this issue? I think the required change for this is straight-forward (set an id for the title) but I don't have any way of testing or validating the fix.

endiliey commented 5 years ago

Actually this is happening to v2 site too, but it isn't a problem there because __docusaurus because it points to the main body div id.

Happening in reactjs.org too, try searching This page is an overview and they'll point to https://reactjs.org/docs/getting-started.html#___gatsby

cc @s-pace from algolia for suggestion.

s-pace commented 5 years ago

👋 @endiliey @parvezakkas

In order to have the best redirection possible, we highly recommend website owner to add a unique id to headings (lvlx selectors defined from the DocSearch configuration) These headings should expose a unique "anchor". Anchors are extracted from HTML attributes (name or id) added to headers that will allow the browser to directly scroll to the right position in the page when clicking a link with a # in it.

From a DocSearch POV, we would need every headings to have a unique id. Not only the h1 tag.

parkas2018 commented 5 years ago

Thanks @s-pace for the explanation.

@endiliey - Do you agree that all docusaurus generated h1 needs to have an id (that seems to be the only one without an id)? I haven't checked the V2 code yet, but for V1 I believe the id needs to be set here on line 241.

https://github.com/facebook/docusaurus/blob/b56adb7ca1e8a0f473b33c00385e902e46c50409/packages/docusaurus-1.x/lib/core/Doc.js#L236-L249

endiliey commented 5 years ago

v2 wont have an id and thats intentional, because the main div body already has id="__docusaurus".

We wont put anchor on both v1 and v2, but I think putting id can make sense for v1 to avoid this scrolling bug. Naturally it will be best to slugify the h1 title, but since the beginning of docusaurus, we never slugify it and it could potentially conflict with other headings defined in markdown

e.g

---
title: hello
---
## Hello

Let's just put id="__docusaurus" here on v1. Its very less likely to see a conflict. Welcoming PR on that

So at the end it will be https://docusaurus.io/docs/en/versioning#__docusaurus

s-pace commented 5 years ago

Thanks for the details @endiliey

Sorry but I don't get what are the drawbacks of having a unique id or name attribute to every headers? It is a W3C recommendation for handling links and redirections.

Having the main div body's id set to "__docusaurus" is a good start. However, the redirection will not be as accurate as having every header accessible from a unique anchor.

Thanks

endiliey commented 5 years ago

Hmmm. Well, to be honest I don't really know why we did so. (had to ask @JoelMarcey as the original author of Docusaurus), but I think its because we actually just don't want the page title as destination anchors. Similar to how the wc3 example don't want Table of Contents to be a destination anchor. That's why the h1 doesnt have an id

https://www.w3.org/TR/html401/struct/links.html image

It's just like https://reactjs.org/docs/getting-started.html image

and https://webpack.js.org/api/node/ image

endiliey commented 5 years ago

P.S Interestingly algolia docs also dont have id for its h1. https://community.algolia.com/docsearch/what-is-docsearch.html image

s-pace commented 5 years ago

Thanks for the prompt reply and the clarification @endiliey

DocSearch Documentation doesn't expose an id for h1 because there is no need to scroll down for these results since h1 are at the top of the page. Every other header does/should expose a unique id or name. By headers, I meant every <hX/> markup, not only h1. Same reasoning applies for webpack. Just notice that this issue is only about <h1/>.

Table of content is always an edge case and I do agree that these elements should not be part of the search either. This one of the reasons why we do recommend to remove them from the DOM before to parse it at the crawling time.

We are going to move our own documentation to docusaurus. I will keep you posted regarding what are the room of improvement in order to provide the best learn-as-you-type experience.

endiliey commented 5 years ago

DocSearch Documentation doesn't expose an id for h1 because there is no need to scroll down for these results since h1 are at the top of the page. Every other header does/should expose a unique id or name. By headers, I meant every markup, not only h1. Same reasoning applies for webpack. Just notice that this issue is only about

.

@s-pace Yep its actually the same for our case too. We didn't put id for h1 because page title are at the top of the page. All other <hX /> will have id. Even h1 header that is not located on the top of the page will have id. (only the most top one that doesnt have it)

We are going to move our own documentation to docusaurus. I will keep you posted regarding what are the room of improvement in order to provide the best learn-as-you-type experience.

Sure thing. On another note, we'd be happy to help dogfood docsearch v3 as well when it came out to all of our v2 users.

parkas2018 commented 5 years ago

@endiliey - Just for clarification, are you saying that v2 of docusaurus does not need an id for the h1 element that is used as the page title? I just checked the v2 site and looks like it has the same issue (I think you mentioned that once):

image

In my opinion, all <hx /> elements should have an id. I think another factor contributing to this scrolling bug is not just the lack of id in the page title that is picked up by search; it's because of the fixed header at the top. If the header is not fixed at the top of the page, we wouldn't see this issue.

I spent some time to find a fix using css but haven't found a proper fix yet. Having said that, I think we need to keep styling and html structure separate as separate concern. Styling should not dictate the html structure, because styling can always change, if needed.

endiliey commented 5 years ago

Just

{this.props.title}

to fix it in v1

Sent from Mail for Windows 10

JoelMarcey commented 5 years ago

Well, to be honest I don't really know why we did so. (had to ask @JoelMarcey as the original author of Docusaurus), but I think its because we actually just don't want the page title as destination anchors. Similar to how the wc3 example don't want Table of Contents to be a destination anchor. That's why the h1 doesnt have an id

I don't recall the exact reason for this, but I believe @endiliey is correct. At the time, we were not going to anchor h1s. It's very possible though it was just an artifact of how we developed the project and wasn't a real conscience decision.

cc @deltice in case he remembers (Hi 👋)

JoelMarcey commented 5 years ago

We are going to move our own documentation to docusaurus. I will keep you posted regarding what are the room of improvement in order to provide the best learn-as-you-type experience.

@s-pace Love this! :)

deltice commented 5 years ago

Well, to be honest I don't really know why we did so. (had to ask @JoelMarcey as the original author of Docusaurus), but I think its because we actually just don't want the page title as destination anchors. Similar to how the wc3 example don't want Table of Contents to be a destination anchor. That's why the h1 doesnt have an id

I don't recall the exact reason for this, but I believe @endiliey is correct. At the time, we were not going to anchor h1s. It's very possible though it was just an artifact of how we developed the project and wasn't a real conscience decision.

cc @deltice in case he remembers (Hi 👋)

Yup, this wasn't really something that had a lot of deliberate thought put into it, we just didn't have anchors for page titles.