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(theme): split admonitions, make swizzle easier, better retrocompatibility #7945

Merged
merged 37 commits into from
Sep 7, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Aug 11, 2022

Motivation

Test Plan

preview

Test links

Deploy preview:

Local:

@slorber slorber added pr: bug fix This PR fixes a bug in a past release. pr: polish This PR adds a very minor behavior improvement that users will enjoy. domain: theme Related to the default theme components labels Aug 11, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 11, 2022
@netlify
Copy link

netlify bot commented Aug 11, 2022

[V2]

Name Link
🔨 Latest commit 4f34062
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6318ba1c2b1dc100090b9b67
😎 Deploy Preview https://deploy-preview-7945--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 Aug 11, 2022

⚡️ Lighthouse report for the deploy preview of this PR

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

@github-actions
Copy link

github-actions bot commented Aug 11, 2022

Size Change: +48 B (0%)

Total Size: 813 kB

Filename Size Change
website/build/assets/js/main.********.js 609 kB +48 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 52.1 kB
website/build/assets/css/styles.********.css 111 kB
website/build/index.html 40.8 kB

compressed-size-action

@Josh-Cena Josh-Cena changed the title refactor(theme): split admonitions, make swizzle easier, handle retrocompatibility better, refactor(theme): split admonitions, make swizzle easier, better retrocompatibility Aug 12, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Maybe add some docs about this? (Wasn't satisfied with our admonition customization docs)

@slorber slorber marked this pull request as draft August 12, 2022 12:03
@slorber
Copy link
Collaborator Author

slorber commented Aug 12, 2022

Yes, it's not finished yet.

I'm also trying to see what can be shared, so that it's easier later to add a Tailwind theme without too much duplication.

I guess we'll also want to share things such as the default labels and icons which are not really theme-specific, but we still want the ability to swizzle to override? 🤷‍♂️

@slorber slorber marked this pull request as ready for review August 24, 2022 18:12
@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Aug 24, 2022
@slorber
Copy link
Collaborator Author

slorber commented Aug 24, 2022

Ready for review.

Satisfied with the current implementation: will merge soon unless I get feedback

@Josh-Cena
Copy link
Collaborator

I'll try reviewing this tonight.

@slorber
Copy link
Collaborator Author

slorber commented Aug 25, 2022

Looks good enough: marked as safe to swizzle.

Note: as we have more "mapping objects" in themes, we should probably add a way to declare a theme module is an object. This way we could support swizzle --wrap such objects. Showing "wrap is forbidden" to the user is a bit confusing considering we encourage wrapping (manually) in our doc

We'll probably do some more admonition options breaking changes in the future but probably not before v4.0. It looks good enough for me to commit to respecting semver

This was referenced Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA domain: theme Related to the default theme components pr: bug fix This PR fixes a bug in a past release. 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.

"Important" admonition has "info" as label