Skip to content

feat(v2): headings anchors#1385

Merged
yangshun merged 3 commits intofacebook:masterfrom
radekmie:master
Apr 24, 2019
Merged

feat(v2): headings anchors#1385
yangshun merged 3 commits intofacebook:masterfrom
radekmie:master

Conversation

@radekmie
Copy link
Contributor

Motivation

An option to quickly copy a section link is essential!

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Check if headings have # anchors with correct id set. Check if clicking right side TOC redirects correctly. Check if navbar is not covering the section after redirection from TOC.

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

docusaurus-bot commented Apr 23, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 34a78a8

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

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Apr 23, 2019

Deploy preview for docusaurus-preview ready!

Built with commit 34a78a8

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

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.

This is such clean and elegant code. Thanks so much! @radekmie

Looks good to me. I'll let @endiliey have a second look since he's more familiar with this part.

We might want to only show the # on hover as it seems to be the more common pattern out there.

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.

Overall LGTM 👍

Just few things:

  1. Lets add a hover property on the hash-link class like v1
  2. Any plan to add test ? Very useful to catch bugs

@yangshun
Copy link
Contributor

yangshun commented Apr 24, 2019

@radekmie I made some changes, hope you don't mind:

  • Appear only on hover
  • Change unit values to rem (our practice moving forwards)

ezgif com-video-to-gif

I'm going to merge this first and release a version. The tests can come later.

@yangshun yangshun merged commit 93894e7 into facebook:master Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants