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(v1): fix page title render issue when referred by search result #1869

Conversation

parkas2018
Copy link
Contributor

@parkas2018 parkas2018 commented Oct 22, 2019

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.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 22, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 22, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 11fea22

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

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 22, 2019

Deploy preview for docusaurus-preview ready!

Built with commit 11fea22

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

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Contributor

endiliey commented Oct 23, 2019 via email

@parkas2018
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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 parkas2018 force-pushed the 1828-fix-page-title-scrolling-behind-nav-bar branch from 991cff1 to 45b5b0f Compare October 24, 2019 19:31
@parkas2018
Copy link
Contributor Author

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

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

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

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

.mainContainer .wrapper .post .postHeader:before, .mainContainer .wrapper .post .postHeaderTitle:before {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@yangshun 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
Copy link
Contributor Author

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 parkas2018 force-pushed the 1828-fix-page-title-scrolling-behind-nav-bar branch from 45b5b0f to 11fea22 Compare November 6, 2019 20:23
@parkas2018
Copy link
Contributor Author

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

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

LGTM. friendly ping @yangshun

@yangshun
Copy link
Contributor

@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
Copy link
Contributor

lex111 commented Nov 17, 2019

@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
Copy link
Contributor Author

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.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@yangshun yangshun merged commit 1bf590e into facebook:master Nov 18, 2019
@endiliey endiliey added the pr: bug fix This PR fixes a bug in a past release. label Nov 21, 2019
@@ -653,6 +653,16 @@ blockquote {
padding: 0;
}

.mainContainer .wrapper .post .postHeader:before,
.mainContainer .wrapper .post .postHeaderTitle:before {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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 @yangshun for the quick fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page titles rendered using H1 should have an ID for permanent link
6 participants