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): clean up sidebars logic; validate generator returns #6596

Merged
merged 5 commits into from
Feb 4, 2022

Conversation

Josh-Cena
Copy link
Collaborator

Motivation

Tweak the sidebar loading order again, after we have a much more complicated logic. This clears all the TODOs. I have also written some brief internal docs on the holistic views of these functions.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

All tests still pass.

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Feb 3, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 3, 2022
\\"collapsible\\": true,
\\"collapsed\\": true
\\"collapsed\\": true,
\\"collapsible\\": true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is very annoying, but Jest wants to reorder object keys in a snapshot even when the value doesn't change...

@netlify
Copy link

netlify bot commented Feb 3, 2022

✔️ [V2]

🔨 Explore the source changes: c23058b

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

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

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 62
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

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

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

Size Change: 0 B

Total Size: 766 kB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 47.2 kB
website/build/assets/css/styles.********.css 105 kB
website/build/assets/js/main.********.js 577 kB
website/build/index.html 36.6 kB

compressed-size-action

@slorber
Copy link
Collaborator

slorber commented Feb 3, 2022

It will be quite hard for me to review this PR 😅 next time try to do it in 2 steps:

  • first refactor without changing any test/behavior => verbose but easy to accept
  • then make some behavior changes => easy to review the little differences (I do see some little changes here, but probably not all)

If you think it's safe to merge, then I trust you considering overall it looks fine

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Feb 3, 2022

first refactor without changing any test/behavior => verbose but easy to accept

I didn't change any behavior; just shifted the steps (unless you count that one extra validate line as a behavior change). If I don't reorganize the workflow there's basically no meaningful refactor.

And, the end behavior is exactly the same. Evidence: no snapshots from outside the sidebars folder have changed

I do plan to self-merge this because I understand the difficulty to review :D Just doing last bits of cleanup

@slorber
Copy link
Collaborator

slorber commented Feb 3, 2022

Understood 👍 go ahead then

I did notice one change, and if there is one I suspect I might not notice all 🤪

For example this snapshot looks different

image

That does not look bad to me to do this change anyway

@Josh-Cena
Copy link
Collaborator Author

Yes, all the default values are applied during post-processing now. This means the generator can return a shape that's very similar to user config and it can still be correctly normalized

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