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

refactor(v2): remove sidebar_label filed from doc metadata file #4863

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented May 28, 2021

Motivation

After merging #4495, all frontmatter fields became part of doc metadata, we have at least one redundant field -- sidebar_label. We can safely remove it from the current metadata, and get it from the frontMatter field instead.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tests passed.

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

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label May 28, 2021
@lex111 lex111 requested a review from slorber as a code owner May 28, 2021 11:59
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 28, 2021
@lex111
Copy link
Contributor Author

lex111 commented May 28, 2021

I also noticed the slug field, which seems to come from frontmatter (?), maybe we should also remove it as a separate field from the doc metadata?

image

In general, are all fields in doc metadata currently used, perhaps we should remove or somehow reduce them?

@netlify
Copy link

netlify bot commented May 28, 2021

✔️ [V2]

🔨 Explore the source changes: e54780d

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60b0db3bd58fc700072e933d

😎 Browse the preview: https://deploy-preview-4863--docusaurus-2.netlify.app

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 64
🟢 Accessibility 97
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4863--docusaurus-2.netlify.app/

@netlify
Copy link

netlify bot commented May 28, 2021

✔️ [V1]

🔨 Explore the source changes: e54780d

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-1/deploys/60b0db3b698bc90007f8ad8d

😎 Browse the preview: https://deploy-preview-4863--docusaurus-1.netlify.app

@github-actions
Copy link

Size Change: 0 B

Total Size: 620 kB

ℹ️ View Unchanged
Filename Size Change
website/build/assets/css/styles.********.css 88.1 kB 0 B
website/build/assets/js/main.********.js 443 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 62.1 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 26.9 kB 0 B

compressed-size-action

@slorber
Copy link
Collaborator

slorber commented Jun 2, 2021

That makes sense thanks!


For the slug, the metadata.slug is not exactly the same as frontMatter.slug (cf getSlug()).

frontmatter slug is optional, but metadata slug is always present and computed using the frontmatter slug

It seems only used in getMainDoc() to test for `doc.slug === '/'

I think we should keep it because we should add better support for docs/index.md => it should have metadata.slug = '/' despite not having explicit frontMatter.slug = '/', and be considered the "home" of a version.

My idea of how this should work to make it easy to understand and maintain:

  • we keep frontmatter 100% untouched
  • when we have to do some computations based on multiple variables (including frontmatter, filenames etc), we compute this once and put it in the doc metadata, so that it can be used in one or more places later without risking to duplicate this computation logic
  • if a metadata value exist, it's better to read it there than on the frontmatter.

sidebar_label can be safely removed from metadata because we don't do any computation on it, so it's always the same value as the frontmatter value, and it's safe to read from the frontmatter directly

@slorber slorber merged commit d72f760 into master Jun 2, 2021
@slorber slorber deleted the lex111/remove-sidebar-label-docmeda branch August 17, 2021 18:03
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: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants