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(content-docs): add tags route as a subroute of version route #6823

Closed
wants to merge 3 commits into from

Conversation

Josh-Cena
Copy link
Collaborator

Motivation

Complete a TODO. This should prevent the entire page from rerendering?

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

The test snapshots are updated

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Mar 3, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 3, 2022
@netlify
Copy link

netlify bot commented Mar 3, 2022

[V2]

Name Link
🔨 Latest commit b2a2f94
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6252bef6c951a00008b0c029
😎 Deploy Preview https://deploy-preview-6823--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 Mar 3, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 45
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

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

@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Size Change: -238 B (0%)

Total Size: 797 kB

Filename Size Change
website/build/assets/js/main.********.js 603 kB -238 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB
website/build/assets/css/styles.********.css 106 kB
website/build/index.html 38.6 kB

compressed-size-action

@Josh-Cena Josh-Cena marked this pull request as draft March 3, 2022 11:03
@Josh-Cena
Copy link
Collaborator Author

I just realized this would create duplicate layouts. With all the wrapperClassName/searchMetadata etc. attached to the current tag page layout, refactoring is not easy...

@slorber
Copy link
Collaborator

slorber commented Mar 3, 2022

IMHO we probably need another "layer"

We probably don't want to have tags reusing the existing DocPage component.

At the same time we want tags to benefit from the version metadata being set in context, and we generally want to keep a shared layout at the very top (ie no unmount/remount on navigation from doc to tag + ability to optimize further later).

A good solution now would be to create an extra component like DocVersionPage that declare layout and provide version in context.

The main problem is docs-doc-page className 😓 we don't have a way to set that class easily from a child component without declaring the layout in the child. Eventually, that might be ok temporarily.

Maybe the page class should rather be set on body directly with Head?

@slorber
Copy link
Collaborator

slorber commented Aug 18, 2022

Superseded by #7966 👍

@slorber slorber closed this Aug 18, 2022
@Josh-Cena Josh-Cena deleted the jc/tag-version-route branch August 18, 2022 11:50
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