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-classic): split theme footer into smaller components + swizzle config #6894

Merged
merged 8 commits into from
Mar 11, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Mar 10, 2022

Motivation

Split theme footer into multiple smaller components to make it easy to add customizations

With the current split, I think it's safe to mark all those components as safe to swizzle wrap/eject?

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

preview is as before

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

netlify bot commented Mar 10, 2022

✔️ [V2]

🔨 Explore the source changes: ac65927

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

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

@github-actions
Copy link

github-actions bot commented Mar 10, 2022

Size Change: +521 B (0%)

Total Size: 792 kB

Filename Size Change
website/build/assets/js/main.********.js 599 kB +521 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB
website/build/assets/css/styles.********.css 105 kB
website/build/index.html 38.7 kB

compressed-size-action

@github-actions
Copy link

github-actions bot commented Mar 10, 2022

⚡️ Lighthouse report for the changes in this PR:

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

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

@netlify
Copy link

netlify bot commented Mar 10, 2022

✔️ [V2]

🔨 Explore the source changes: 1d78ee4

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

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

@slorber
Copy link
Collaborator Author

slorber commented Mar 11, 2022

@Josh-Cena I'm comfortable merging this PR now

As it's public API in which we commit to have no breaking change on majors, let me know if you have any concern on naming/organisation

</footer>
<FooterLayout
style={style}
links={links && links.length > 0 && <FooterLinks links={links} />}
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 really like this style... Can we just pass links down and render it in FooterLayout?

I know that it means FooterLayout will now be tied to those sub-components. TBH, I'm not even clear what the value of this abstraction is, because users will likely not swizzle/change this generic component's props without altering the other ones' anyways.

Copy link
Collaborator Author

@slorber slorber Mar 11, 2022

Choose a reason for hiding this comment

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

It is important for me to isolate the macro layout of a theme component in its own abstraction, mostly taking ReactNode props.

It's also a good example of what I'd like all theme "big sections" to look like.

I'd also like the "entry" of a "big section" to be simple and free of any Infima class or low-level details (ie not using any DOM component, className, listeners...)

Users should be able to easily do things like put the logo/copyright above the links, add extra sections etc without managing much code complexity (ie dealing with simple vs multi-column layouts and type guards).

I agree it may look a bit overkill though (as the code complexity it hides is not so complex) but I'd like to keep it this way, as an example, and for consistency with what we'll do in other places (where it will be less overkill)


Layout components are a common React pattern. They can also have some performance benefits (see https://overreacted.io/before-you-memo/) make it easier to implement other themes in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I see 👍

Should we move the links && links.length > 0 && to FooterLinks then? This will make this component cleaner and also more robust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doing so is not the best idea and could lead to antipatterns. In general it's preferable if the component itself doesn't decide if it renders something or not, because it makes a composition with layouts more complex and error-prone.

For example, look at the logo here, we add a margin if the logo is defined:

image

Now if we always pass a ReactNode as props, the prop is always defined and this potentially leads to a margin being added uselessly around a null element.

In general, I'd like to avoid components rendering null (preferring the parent to not render the component in the first place) unless it's clear it's safe to do so. For example, it looks quite safe for the top-level Footer component to render null

Copy link
Collaborator

Choose a reason for hiding this comment

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

links && links.length > 0 && <FooterLinks links={links} /> where FooterLinks evaluates to null is the same as links being null, no? 🤷‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure to understand what you mean here 😅

Imagine the user swizzling the layout wants to add some margin around the links, he should be able to do so:

{links && <ExtraMargin>{links}</ExtraMargin>}

Now if links is always a ReactNode in the layout component, then the extra margin is always added, even if links is null

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see... Okay, makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

going to merge then

if you have improvement ideas, don't hesitate to submit PRs before 2.0

@slorber slorber merged commit 1efc6c6 into main Mar 11, 2022
@slorber slorber deleted the slorber/theme-footer-split branch March 11, 2022 13:55
@slorber slorber mentioned this pull request Mar 16, 2022
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