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

feat(core): brand new swizzle CLI experience #6243

Merged
merged 111 commits into from
Feb 25, 2022
Merged

feat(core): brand new swizzle CLI experience #6243

merged 111 commits into from
Feb 25, 2022

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Jan 2, 2022

Motivation

  • Major refactor, brand new DX
  • New swizzle documentation
  • CLI is interactive
  • --eject and --wrap options
  • --list option: exit with list when no extra args are given; otherwise, enter an interactive choosing mode.
  • Better error messages: they are now always what should intuitively be reported
  • Completed the swizzle infrastructure of other themes.

This would be a preparation for #6114.

Closes #5380

Docs

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Local tests

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Jan 2, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 2, 2022
@netlify
Copy link

netlify bot commented Jan 2, 2022

✔️ [V2]

🔨 Explore the source changes: 3da40a8

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

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

@github-actions
Copy link

github-actions bot commented Jan 2, 2022

⚡️ Lighthouse report for the changes in this PR:

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

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

@github-actions
Copy link

github-actions bot commented Jan 2, 2022

Size Change: +471 B (0%)

Total Size: 783 kB

Filename Size Change
website/.docusaurus/globalData.json 48.2 kB +134 B (0%)
website/build/assets/js/main.********.js 591 kB +337 B (0%)
ℹ️ View Unchanged
Filename Size
website/build/assets/css/styles.********.css 106 kB
website/build/index.html 37.6 kB

compressed-size-action

@Josh-Cena Josh-Cena marked this pull request as ready for review January 3, 2022 04:48
@Josh-Cena
Copy link
Collaborator Author

Let's implement --eject and --wrap in a separate release.

@Josh-Cena
Copy link
Collaborator Author

LGTM @slorber I think we can merge it today

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -4,9 +4,11 @@ description: Customize your site's appearance through creating your own theme co

# Swizzling

When [styling with CSS](./styling-layout.md) is not enough, **swizzling** comes into play.
In this section, we will introduce how customization of layout is done in Docusaurus.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not particularly about layouts, you can swizzle design system components too (or at least you will be able to)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Design systems are still "layouts"... Maybe "appearance" is better?

@@ -80,7 +76,7 @@ export async function eject({

await fs.ensureDir(toPath);

async function copyFile(sourceFile: string) {
const createdFiles = await Promise.all(filesToCopy.map(async (sourceFile: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this kind of separation is done on purpose, it's clean code to separe low-level from high-level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes scanning code a bit harder for me for little reason... I prefer to see that await Promise.all() as one syntactic construct about working on an array of promises, so this code still reads idiomatic to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes scanning code harder for me and many others

In the future, I'd prefer if you apply clean code rules even if you don't agree with this one

http://principles-wiki.net/principles:single_level_of_abstraction
https://www.tripled.io/27/09/2016/Levels-of-abstraction/


> Déja vu...?

This section is similar to [Styling and Layout](./styling-layout.md), but this time, we are going to write actual React code and go deeper into the internals instead of playing with stylesheets. We will talk about a central concept in Docusaurus customization: **swizzling**, swizzling allows **deeper site customizations** through **React components**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤷‍♂️ I don't know, I prefer the former shorter version. Why make the text longer if it does not convey more info?

We don't go deeper into the internals, it is an official public feature and only unsafe components are internal

And not sure it's ok to say "playing with stylesheets" 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Writing code is considered "internal". This is the only section in the Guides that asks readers to write actual code.

@@ -120,6 +120,10 @@ function success(msg: unknown, ...values: InterpolatableValue[]): void {
);
}

function newLine(): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we remove this?

I like adding linebreaks in CLI to make output more readable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IDK, I found one place where it makes some sense, but mostly we can just write logger.info('\n...\n')

@slorber slorber merged commit 39b66d8 into main Feb 25, 2022
@slorber slorber deleted the jc/swizzle-rework branch February 25, 2022 13:13
@slorber slorber changed the title feat(core): rework swizzle CLI feat(core): brand new swizzle CLI experience Feb 25, 2022
@Josh-Cena Josh-Cena mentioned this pull request Mar 16, 2022
2 tasks
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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.0] RFC: docusaurus swizzle --wrap/copy Update swizzleAllowedComponents
3 participants