fix(v1): fix page title render issue when referred by search result #1869

parkas2018 · 2019-10-22T15:09:43Z

When Algolia DocSearch query finds a match for a page's title, it attempts to generate
a permalink. Because the page title element (h1) does not have an id, Algolia
generates uses the id from closes parent element. Because of this, the page title
scrolls to a position that is slightly overlayed by the fixed top navigation bar.

This fix sets an id for the page title so that the search result is able to generate
a more accurate permalink.

closes #1828

Motivation

This is a wide-spread bug that is affecting everyone and the required change is relatively small.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

The original issue is caused by algolia attempting to auto generate a permalink for the page title that does not actually have a specific id. That's why it's not possible to test this change locally and verify the fix. But, based on discussions in #1828, it's recommended by algolia docsearch to have an id for the page title so that algolia can produce the best redirect/link for the search result.

@endiliey - As per your comment in the above mentioned issue, I didn't make changes in V2 code, but I still see the same issue in V2 doc site used for Docusaurus documentation. I'm not fully clear on why we don't want this fix in V2 code. Please feel free to let me know if you'd like me to make any changes.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

docusaurus-bot · 2019-10-22T15:14:37Z

Deploy preview for docusaurus-2 ready!

Built with commit 11fea22

https://deploy-preview-1869--docusaurus-2.netlify.com

docusaurus-bot · 2019-10-22T15:15:44Z

Deploy preview for docusaurus-preview ready!

Built with commit 11fea22

https://deploy-preview-1869--docusaurus-preview.netlify.com

endiliey

In v2 the closest parent element with id is the main body div, which has id=__docusaurus while v1 closest parent element is docsNav, which causes the scrolling issue

After re-looking at it again, i think adding id wont fix the issue. Its still the css problem due to fixed header

For example: try https://deploy-preview-1869--docusaurus-preview.netlify.com/docs/en/installation#__docusaurus

wdyt ?

parkas2018 · 2019-10-23T14:17:52Z

After re-looking at it again, i think adding id wont fix the issue. Its still the css problem due to fixed header

You're right. I think some adjustments in the css is needed similar to the anchored headers. Let me look into it. I'll push another update as soon as I'm able to look into it.

endiliey · 2019-10-23T14:37:47Z

Sure thing. Thanks a lot by the way :)

…

Sent from my iPhone

On 23 Oct 2019, at 9:18 PM, PA ***@***.***> wrote: After re-looking at it again, i think adding id wont fix the issue. Its still the css problem due to fixed header You're right. I think some adjustments in the css is needed similar to the anchored headers. Let me look into it. I'll push another update as soon as I'm able to look into it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

parkas2018 · 2019-10-23T18:45:25Z

I can't seem to find a proper css fix. But I noticed that adjusting the code to this actually fixes the scrolling issue (no need for css code change):

            <h1 className="postHeaderTitle">
              <a className="anchor" aria-hidden="true" id="__docusaurus"></a>
              {this.props.title}
            </h1>

Only problem is that I get linter errors about the empty html <a> element. I personally don't like the empty element either but looks like that's how we have all the other header elements being set.

Any suggestions? Do you have any css tricks for this :)

endiliey · 2019-10-24T00:08:45Z

Its because “anchor” css has relative position and top edge. Check the main.css and you’ll be able to see the css for it

parkas2018 · 2019-10-24T15:06:22Z

Oh I know :). I've seen the css in main.css file that is applied to anchors but just that by itself doesn't fix the post title. I'll take a look a bit more in case I missed something.

parkas2018 · 2019-10-24T19:35:06Z

I think I've found the CSS fix that was needed for this PR. Hopefully everything looks good to be merged 🤞 😄

Found the following article quite helpful. Especially I like that this fix does not require using unnecessary html elements. The html structure is remains cleaner. Perhaps this can be adopted for the V2 site when generating the table of contents.

https://css-tricks.com/hash-tag-links-padding/#article-header-id-1

packages/docusaurus-1.x/lib/core/Doc.js

yangshun

Comments:

Can we generate an id value for the <h1> based on the title? Doesn't seem right to use the same hardcoded id for the title on all pages
Why not target the specific class added to the doc header and add similar position offset CSS like we did for the page's h2 and h3? If the class doesn't exist, we could add one.

With the above, the issue should be fixed in both v1 and v2 right?

cc @endiliey

yangshun · 2019-10-27T21:52:56Z

packages/docusaurus-1.x/lib/static/css/main.css

@@ -653,6 +653,15 @@ blockquote {
  padding: 0;
 }

+.mainContainer .wrapper .post .postHeader:before, .mainContainer .wrapper .post .postHeaderTitle:before {


Separate selectors should be on its own line.

We shouldn't be writing the CSS like this. With such a specific hierarchy, this is bound to break if we change the DOM structure. Can we use a less-specific selector?

I was actually following the same hierarchy that was already in place for styling the post title so that it's all consistent. If you still want me to use "less-specific" selector, I can go ahead and push another update.

Separate selectors should be on its own line.

@yangshun - Just for clarification, did you mean writing it like this?

.mainContainer .wrapper .post .postHeader:before, .mainContainer .wrapper .post .postHeaderTitle:before { .... }

We shouldn't be writing the CSS like this. With such a specific hierarchy, this is bound to break if we change the DOM structure. Can we use a less-specific selector?

As I mentioned in my previous reply, I'm following the CSS structure that was there initially. Do you still want me to update my changes to use less-specific selector?

Just for clarification, did you mean writing it like this?

I'm not yangshun, but yes, that’s exactly what was meant.

As I mentioned in my previous reply, I'm following the CSS structure that was there initially. Do you still want me to update my changes to use less-specific selector?

Fair enough, better following current CSS structure.

Thanks for the confirmation @lex111 . I'll update the css so that different selectors are on separate line.

parkas2018 · 2019-10-28T14:27:29Z

Comments:

Can we generate an id value for the <h1> based on the title? Doesn't seem right to use the same hardcoded id for the title on all pages

Why not target the specific class added to the doc header and add similar position offset CSS like we did for the page's h2 and h3? If the class doesn't exist, we could add one.

With the above, the issue should be fixed in both v1 and v2 right?

cc @endiliey

@yangshun - Thank you for reviewing this PR. The main issue is not just related to CSS. Algolia search needs to generate a permalink using id of the headers if it matches the search query. You can see more detailed conversation on both of your comments in #1828. Hope that helps.

When Algolia DocSearch query finds a match for a page's title, it attempts to generate a permalink. Because the page title element (`h1`) does not have an `id`, Algolia generates uses the `id` from closes parent element. Because of this, the page title scrolls to a position that is slightly overlayed by the fixed top navigation bar. This fix sets an `id` for the page title so that the search result is able to generate a more accurate permalink.

…an anchor The post title can sometimes be referred by an anchor using the "id" of that element. In that case the title will be automatically be the first element within the viewport. Since we have a header fixed at the top of the view port, the title becomes hidden or not visible. That's why some css adjustments are needed so that if any user ends up with a link to a page that is referring to the post title (i.e. auto generated anchor link by algolia DocSearch). The css code uses pseudo element `:before` to make the adjustments. Details on this can be found in the following article: https://css-tricks.com/hash-tag-links-padding/

parkas2018 · 2019-11-14T14:42:16Z

@yangshun, @lex111, @endiliey - Would you be able to review this please? Not sure if there are still outstanding concerns.

lex111

LGTM. friendly ping @yangshun

yangshun · 2019-11-17T11:22:01Z

@parvezakkas Sorry for the delay in review. I'm not seeing the fix in the Netlify preview though. Going to https://deploy-preview-1869--docusaurus-preview.netlify.com/docs/en/navigation#docsNav still makes the h1 slightly obscured.

lex111 · 2019-11-17T11:29:25Z

@yangshun I think you need to wait for the reindexing of Algolia (after merge this PR), because in current PR own id is added to h1. https://github.com/facebook/docusaurus/pull/1869/files#diff-e025b6c2db1283b854d503afb8d88f57R241

Therefore, you need to look at https://deploy-preview-1869--docusaurus-preview.netlify.com/docs/en/site-preparation#__docusaurus instead of #docsNav

parkas2018 · 2019-11-18T19:00:12Z

Thanks @yangshun for reviewing this. You'll need to check the link that @lex111 shared (ending with #__docusaurus instead of #docsNav) because the preview site is not indexed by Algolia.

yangshun

Awesome, thank you!

endiliey · 2019-11-22T03:12:56Z

packages/docusaurus-1.x/lib/static/css/main.css

@@ -653,6 +653,16 @@ blockquote {
  padding: 0;
 }

+.mainContainer .wrapper .post .postHeader:before,
+.mainContainer .wrapper .post .postHeaderTitle:before {


This caused a CSS regression bug in which Blog post "Read more" doesnt work anymore

https://deploy-preview-1869--docusaurus-preview.netlify.com/blog/

Thank you @yangshun for the quick fix

parkas2018 requested a review from endiliey as a code owner October 22, 2019 15:09

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 22, 2019

endiliey reviewed Oct 23, 2019

View reviewed changes

parkas2018 force-pushed the 1828-fix-page-title-scrolling-behind-nav-bar branch from 991cff1 to 45b5b0f Compare October 24, 2019 19:31

lex111 suggested changes Oct 25, 2019

View reviewed changes

packages/docusaurus-1.x/lib/core/Doc.js Show resolved Hide resolved

yangshun suggested changes Oct 27, 2019

View reviewed changes

yangshun changed the title ~~fix(v1): Fix page title render issue when referred by search result~~ fix(v1): fix page title render issue when referred by search result Oct 27, 2019

parkas2018 requested review from lex111 and yangshun November 6, 2019 16:21

parkas2018 added 3 commits November 6, 2019 15:20

Adjust CSS so that different selectors are on separate lines

11fea22

parkas2018 force-pushed the 1828-fix-page-title-scrolling-behind-nav-bar branch from 45b5b0f to 11fea22 Compare November 6, 2019 20:23

parkas2018 requested a review from endiliey November 8, 2019 15:32

lex111 approved these changes Nov 14, 2019

View reviewed changes

lex111 mentioned this pull request Nov 17, 2019

fix(v2): adjust first-level heading offset #2005

Merged

yangshun approved these changes Nov 18, 2019

View reviewed changes

yangshun merged commit 1bf590e into facebook:master Nov 18, 2019

endiliey added the pr: bug fix This PR fixes a bug in a past release. label Nov 21, 2019

endiliey mentioned this pull request Nov 21, 2019

Escape angle brackets in configuration jestjs/jest#9189

Merged

endiliey reviewed Nov 22, 2019

View reviewed changes

endiliey mentioned this pull request Nov 22, 2019

fix (v1): fix broken relative links related to --skip-next-release build option #1816

Closed

yangshun mentioned this pull request Nov 22, 2019

fix(v1): apply negative margin to docs heading only #2028

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(v1): fix page title render issue when referred by search result #1869

fix(v1): fix page title render issue when referred by search result #1869

parkas2018 commented Oct 22, 2019 •

edited

Loading

docusaurus-bot commented Oct 22, 2019 •

edited

Loading

docusaurus-bot commented Oct 22, 2019 •

edited

Loading

endiliey left a comment •

edited

Loading

parkas2018 commented Oct 23, 2019

endiliey commented Oct 23, 2019 via email

parkas2018 commented Oct 23, 2019

endiliey commented Oct 24, 2019

parkas2018 commented Oct 24, 2019

parkas2018 commented Oct 24, 2019

yangshun left a comment •

edited

Loading

yangshun Oct 27, 2019

parkas2018 Oct 28, 2019

parkas2018 Nov 5, 2019

lex111 Nov 6, 2019

parkas2018 Nov 6, 2019

parkas2018 commented Oct 28, 2019

parkas2018 commented Nov 14, 2019

lex111 left a comment

yangshun commented Nov 17, 2019

lex111 commented Nov 17, 2019

parkas2018 commented Nov 18, 2019

yangshun left a comment

endiliey Nov 22, 2019

parkas2018 Nov 22, 2019

fix(v1): fix page title render issue when referred by search result #1869

fix(v1): fix page title render issue when referred by search result #1869

Conversation

parkas2018 commented Oct 22, 2019 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

docusaurus-bot commented Oct 22, 2019 • edited Loading

docusaurus-bot commented Oct 22, 2019 • edited Loading

endiliey left a comment • edited Loading

Choose a reason for hiding this comment

parkas2018 commented Oct 23, 2019

endiliey commented Oct 23, 2019 via email

parkas2018 commented Oct 23, 2019

endiliey commented Oct 24, 2019

parkas2018 commented Oct 24, 2019

parkas2018 commented Oct 24, 2019

yangshun left a comment • edited Loading

Choose a reason for hiding this comment

yangshun Oct 27, 2019

Choose a reason for hiding this comment

parkas2018 Oct 28, 2019

Choose a reason for hiding this comment

parkas2018 Nov 5, 2019

Choose a reason for hiding this comment

lex111 Nov 6, 2019

Choose a reason for hiding this comment

parkas2018 Nov 6, 2019

Choose a reason for hiding this comment

parkas2018 commented Oct 28, 2019

parkas2018 commented Nov 14, 2019

lex111 left a comment

Choose a reason for hiding this comment

yangshun commented Nov 17, 2019

lex111 commented Nov 17, 2019

parkas2018 commented Nov 18, 2019

yangshun left a comment

Choose a reason for hiding this comment

endiliey Nov 22, 2019

Choose a reason for hiding this comment

parkas2018 Nov 22, 2019

Choose a reason for hiding this comment

parkas2018 commented Oct 22, 2019 •

edited

Loading

docusaurus-bot commented Oct 22, 2019 •

edited

Loading

docusaurus-bot commented Oct 22, 2019 •

edited

Loading

endiliey left a comment •

edited

Loading

yangshun left a comment •

edited

Loading