-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Conversation
✔️ [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 |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6283--docusaurus-2.netlify.app/ |
Size Change: +598 B (0%) Total Size: 674 kB
ℹ️ View Unchanged
|
Still doesn't fix #6235😞 Not a blocker though, current one looks good enough |
👍 may be something else then 😅 |
@slorber Curious why you decided to remove MainHeading? I was targeting this item specifically. In my view it had semantic importance. |
@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 Note that you can still wrap the existing heading component and provide your own implementation if the |
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 Since Maybe I'm missing something? I guess I was expecting Heading and MainHeading to have been separated. |
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? |
Humm.... You're right. Let me take some time to step back and reevaluate. |
Have you figured this out @roydukkey ? |
@slorber I guess not exactly. At least not in a preferable manner. Essentially I believe my use case is opposite to the This is the only way I can imagine achieving this with the change on this PR:
I went looking for some other existing similar use cases:
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. |
There was a problem hiding this 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
@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 |
Breaking changes
Some theme components were refactored. If you swizzled them, you might need to upgrade them:
Motivation
Theme components should only have a default export, it makes the
swizzle --wrap
possibleSee 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