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(v2): unify anchor behavior #2162

Merged
merged 4 commits into from
Jan 23, 2020
Merged

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Dec 30, 2019

Motivation

First of all, remove large margin on all screens (see test plan) when following a link from anchor. Further, when you open the referenced link in the browser, the header is automatically hidden, similarly when switching to another anchor on this page.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Prerequisite: open link with anchor, eg https://v2.docusaurus.io/docs/installation#project-structure

Before After
image image

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

docusaurus-bot commented Dec 30, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 6bd6d35

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

@lex111
Copy link
Contributor Author

lex111 commented Jan 1, 2020

@yangshun friendly ping

@lex111
Copy link
Contributor Author

lex111 commented Jan 4, 2020

Anybody here? 😃

lex111 referenced this pull request Jan 7, 2020
* Tribute to Endi

* Add missing twitter link
@bravo-kernel
Copy link
Contributor

@lex111 I just tried deeplinking against the preview using https://deploy-preview-2162--docusaurus-2.netlify.com/docs/cli/#docusaurus-swizzle and must say that I would prefer to see the anchor text/title, looking only at the text below it makes me (always) scroll up a little to see if things went well.

I also noticed that the Header component does not add anchors which is probably a separate request but very useful for autogenerated pages that ref the H1 anchor.

@bravo-kernel
Copy link
Contributor

This is what i see when using mentioned deeplink:

image

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.

Does this preserve functionality for sites without the hideable navbar?

@lex111
Copy link
Contributor Author

lex111 commented Jan 12, 2020

This is what i see when using mentioned deeplink:

It is strange, there shouldn’t be one, header should be hidden when following the link with anchor. I changed something, so try again.

Does this preserve functionality for sites without the hideable navbar?

Ah, yeah, did it. ✔️

@bravo-kernel
Copy link
Contributor

@lex111 thanks for the quick action. The deeplink now opens as expected 💃

image

@bravo-kernel
Copy link
Contributor

Can you confirm that the H1 element on the docs pages are indeed missing anchors? I understand that this is probably a separate issue but I would like confirmation before I create a ticket.

@lex111
Copy link
Contributor Author

lex111 commented Jan 12, 2020

@bravo-kernel h1 elements do not contain an anchor, and why is it needed? These headers are at the very top of the page, so there is no point in the anchors, right?

@bravo-kernel
Copy link
Contributor

If we use hide_title: false the (markdown) H1 that gets rendered does have anchors.

This is useful/required for e.g. pages that autogenerate documentation like jsdoc. They create references/links to the main class and that link points to the H1 anchor (liike it would also do in e.g. github md view).

I think it would be best if markdown H1 and Heading would produce the same consistent and predictable behavior.

@bravo-kernel
Copy link
Contributor

No big thing btw, just wanted to share the finding, I can work around it for jsdoc

@bravo-kernel
Copy link
Contributor

@lex111 I read more about the H1 anchor and agree with your statement; the Heading component is probably correct here (no anchor needed).

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Jan 21, 2020
@lex111 lex111 requested a review from yangshun January 21, 2020 21:27
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.

I tested locally with non-hidable navbar too. Looks good but now the fake anchor gets highlighted when I click from the sidebar. See example:

Screen Shot 2020-01-23 at 10 47 04 PM

Self-merge after fixing.

@lex111 lex111 merged commit 1555394 into facebook:master Jan 23, 2020
@lex111 lex111 deleted the unify-anchor-behavior branch January 23, 2020 16:06
@bravo-kernel
Copy link
Contributor

Thanks 👏

@lex111 lex111 restored the unify-anchor-behavior branch April 18, 2020 13:54
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.

5 participants