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): apply import/no-named-export eslint rule #6283

Merged
merged 6 commits into from
Jan 7, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jan 6, 2022

Breaking changes

Some theme components were refactored. If you swizzled them, you might need to upgrade them:

  • Heading
  • DocSidebarItem
  • DocNavbar

Motivation

Theme components should only have a default export, it makes the swizzle --wrap possible

See also #5380 (comment)

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

same behavior as before, it should build and preview should look the same

@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Jan 6, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 6, 2022
@netlify
Copy link

netlify bot commented Jan 6, 2022

✔️ [V2]

🔨 Explore the source changes: 25e68b0

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

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

@github-actions
Copy link

github-actions bot commented Jan 6, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 98
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@github-actions
Copy link

github-actions bot commented Jan 6, 2022

Size Change: +598 B (0%)

Total Size: 674 kB

Filename Size Change
website/build/assets/js/main.********.js 501 kB +598 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 41.6 kB
website/build/assets/css/styles.********.css 102 kB
website/build/index.html 29.6 kB

compressed-size-action

@Josh-Cena
Copy link
Collaborator

Still doesn't fix #6235😞 Not a blocker though, current one looks good enough

@slorber
Copy link
Collaborator Author

slorber commented Jan 7, 2022

👍 may be something else then 😅

@slorber slorber merged commit 3bc63b2 into main Jan 7, 2022
@slorber slorber deleted the slorber/theme-no-named-export branch January 7, 2022 13:44
@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Jan 7, 2022
@roydukkey
Copy link
Contributor

@slorber Curious why you decided to remove MainHeading? I was targeting this item specifically. In my view it had semantic importance.

@slorber
Copy link
Collaborator Author

slorber commented Jan 7, 2022

@roydukkey it's explained in the linked issue #5380

We want a theme to be mostly React UI components, all having a single default export. This is important for a docusaurus swizzle --wrap feature we are working on.

Note that you can still wrap the existing heading component and provide your own implementation if the as prop is h1 => restore your old behavior

@roydukkey
Copy link
Contributor

roydukkey commented Jan 7, 2022

I cannot just (only) wrap the existing Heading component to achieve what I was doing with MainHeading, because DocCategoryGeneratedIndexPage and DocItem no longer contain a logical and targetable difference for the heading element.

As it was before <MainHeading /> was unique (served a specific purpose; logically) from <Heading as="h1" />.

Since <Heading as="h1" /> is used elsewhere (markdown), this change requires me to wrap the Heading component, but also and much less desirably, it requires me to eject the DocCategoryGeneratedIndexPage and DocItem components in order to reach into the JSX to make these heading unique again.

Maybe I'm missing something? I guess I was expecting Heading and MainHeading to have been separated.

@slorber
Copy link
Collaborator Author

slorber commented Jan 7, 2022

I see

Please describe exactly what you are trying to do/customize in terms of features (not in terms of problems) so that I can figure out the best solution to enable you to do what you want.

Should we have some kind of DocTitle component?

@roydukkey
Copy link
Contributor

roydukkey commented Jan 7, 2022

(not in terms of problems)

Humm.... You're right.

Let me take some time to step back and reevaluate.

@slorber
Copy link
Collaborator Author

slorber commented Jan 12, 2022

Have you figured this out @roydukkey ?

@roydukkey
Copy link
Contributor

@slorber I guess not exactly. At least not in a preferable manner. Essentially I believe my use case is opposite to the DocItemFooter component, since I'm trying to get content inserted before the main heading, which results in @sass-fairy/list as seen here.

This is the only way I can imagine achieving this with the change on this PR:

  1. Wrap DocVersionBadge to insert my content before the heading
  2. Style styles.docItemContainer to be display: flex
  3. Style DocVersionBadge and TOCCollapsible with order: -1
  4. Hope this doesn't break existing theme styles
  5. Live with the unfavourable consequences on SEO

I went looking for some other existing similar use cases:

  1. C# List<T>.Sort() Doc

    Adds two pieces of content: "Reference" and "Is this page helpful?".

    • Similar in the fact supporting content targets the main heading.
    • Different in the fact supporting content is inserted after the main heading rather than before it.

    This actually cannot be achieved just with wrapping.

    1. Header here is not unique
    2. <DocContent /> is not a theme component.
  2. w3schools.com Array Sort()

    Could potentially use the <DocPaginator /> to achieve similar functionality, but... the same

  3. What @Josh-Cena has done here, viewed here...

    ...before this PR, he could have achieved without needing to add the MDX to every page.

I guess where I'm ultimately leaning is that it is fairly common that there are supporting pieces of content the generally get placed around the main heading of a document, either before or after.

Copy link

@csestito csestito left a comment

Choose a reason for hiding this comment

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

accidentally submitted here instead of another review

@slorber
Copy link
Collaborator Author

slorber commented Feb 2, 2022

@roydukkey please report your potential customization usecase here and we'll figure out a good way to allow this soon #5468

Try to focus on the outcome and explain why you want it

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: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants