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(docs): forward doc frontMatter.sidebar_custom_props to linking sidebar category #7638

Merged
merged 2 commits into from Jun 16, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 16, 2022

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

While investigating #7598, I realized that we don't always forward sidebar_custom_props to the sidebar element

If a category has a doc link, then the doc frontMatter.sidebar_custom_props should be used as category customProps.

This is consistent with the behavior we have for autogenerated sidebars already:

      const customProps =
        categoryMetadata?.customProps ?? categoryLinkedDoc?.customProps;

Test Plan

Quick fix, local test only.

I think props.ts and generator.ts are not the ideal place to handle that. The "custom props forwarding" is spread across multiple parts of the codebase.

We should probably refactor all that and handle all the custom props forwarding in postProcessor.ts, sharing the same logic between autogenerated items and explicit items. And add good tests there.

Also maybe we want to merge custom props instead of ?? ?

Any opinion on that refactor @Josh-Cena ?

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jun 16, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 16, 2022
@netlify
Copy link

netlify bot commented Jun 16, 2022

[V2]

Name Link
🔨 Latest commit 1c4d432
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62ab613a46172c0008867d87
😎 Deploy Preview https://deploy-preview-7638--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

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 95 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 89 🟢 100 🟢 100 🟢 100 🟢 90 Report

@netlify
Copy link

netlify bot commented Jun 16, 2022

[V2]

Name Link
🔨 Latest commit 54007f1
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62ab6265f85f12000a68ab38
😎 Deploy Preview https://deploy-preview-7638--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

Size Change: 0 B

Total Size: 801 kB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 52.6 kB
website/build/assets/css/styles.********.css 106 kB
website/build/assets/js/main.********.js 603 kB
website/build/index.html 38.9 kB

compressed-size-action

@slorber slorber merged commit 6df379c into main Jun 16, 2022
@slorber slorber deleted the slorber/category-index-show-category-description branch June 16, 2022 17:20
@Josh-Cena
Copy link
Collaborator

We should probably refactor all that and handle all the custom props forwarding in postProcessor.ts, sharing the same logic between autogenerated items and explicit items. And add good tests there.

I think so, yeah. sidebar_label is applied in props.ts, and sidebar_class_name is never applied. We need to make this consistent.

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.

None yet

3 participants