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.

Sign up for GitHub

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

Jump to bottom

fix(content-docs): use category index convention to infer document title #8052

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(content-docs): use category index convention to infer document title #8052

wants to merge 1 commit into from

Conversation

MarkShawn2020
Copy link
Contributor

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

I found if I have file path like xx/yy/zz/index.md or xx/yy/zz/readme.md without a 'tittle' frontmatter or '#1' title in the document, then the title of this file shown in the sidebar would be index or readme, which is not desired, since the directory name zz may be more suitable.

Test Plan

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 6, 2022
@netlify
Copy link

netlify bot commented Sep 6, 2022

✅ [V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 353a125
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63173a4a2937a20008490501
😎 Deploy Preview https://deploy-preview-8052--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 73 🟢 98 🟢 100 🟢 100 🟠 80 Report
/docs/installation 🟠 75 🟢 100 🟢 100 🟢 100 🟢 90 Report

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Sep 7, 2022
@slorber slorber changed the title enhance: fallback sidebar title to dirname if index/readme fix(content-docs): use category index convention to infer document title Sep 7, 2022
slorber
slorber requested changes Sep 7, 2022
View reviewed changes
Copy link
Collaborator

@slorber slorber left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, that looks like a useful change that I'll classify as a bug fix (and not a breaking change)

Note we already have a convention to detect "category index documents", see

// By convention, Docusaurus considers some docs are "indexes":
// - index.md
// - readme.md
// - <folder>/<folder>.md
//
// This function is the default implementation of this convention
//
// Those index docs produce a different behavior
// - Slugs do not end with a weird "/index" suffix
// - Auto-generated sidebar categories link to them as intro
export const isCategoryIndex: CategoryIndexMatcher = ({
  fileName,
  directories,
}): boolean => {
  const eligibleDocIndexNames = [
    'index',
    'readme',
    directories[0]?.toLowerCase(),
  ];
  return eligibleDocIndexNames.includes(fileName.toLowerCase());
};

It makes sense to find a way to reuse the same logic instead of duplicating it.

We also want to cover such subtle behaviors with unit tests

Also note that with autogeneated sidebars, folders might have a number prefix for ordering reasons, and you may want to strip that number in the infered title: /xx/yy/03-zz/index.md should infer to zz and not 03-zz? (see impl of getSlug() for inspiration)

@slorber slorber marked this pull request as draft September 7, 2022 11:44
@MarkShawn2020
Copy link
Contributor Author

MarkShawn2020 commented Sep 8, 2022 •
edited
Loading

Note we already have a convention to detect "category index documents", see

Good job！

We also want to cover such subtle behaviors with unit tests

Sorry to admit that I have few ideas on how to write unit tests for directory strucurues.

Also note that with autogeneated sidebars, folders might have a number prefix for ordering reasons, and you may want to strip that number in the infered title: /xx/yy/03-zz/index.md should infer to zz and not 03-zz? (see impl of getSlug() for inspiration)

Yep!

@Josh-Cena Josh-Cena marked this pull request as ready for review September 8, 2022 12:55
@Josh-Cena Josh-Cena marked this pull request as draft September 30, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slorber slorber slorber requested changes

@lex111 lex111 Awaiting requested review from lex111

@Josh-Cena Josh-Cena Awaiting requested review from Josh-Cena Josh-Cena is a code owner

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarkShawn2020 @slorber @facebook-github-bot