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): clean up CSS of doc sidebar item #6622

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 6, 2022

Motivation

After a cursory look of the final source code produced by Docusaurus, I found that there are two redundant CSS classes for doc sidebar items. So it's safe to remove these classes and the related CSS code.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview

Related PRs

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Feb 6, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 6, 2022
@@ -6,18 +6,6 @@
*/

@media (min-width: 997px) {
.menuLinkText {
cursor: initial;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the href attribute is missing in a link, the default cursor is used, so there is no need for this class.

}

.menuLinkText:hover {
background: none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, we don't need this CSS class after introducing menu__list-item-collapsible.

@@ -18,6 +18,7 @@ const sidebars = {
type: 'generated-index',
},
collapsed: false,
collapsible: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's disable collapsing for this category as a dogfooding approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have another section to dogfood now, maybe you can move these tests there?

(one with/without link and with/without collapsible?)

https://deploy-preview-6622--docusaurus-2.netlify.app/tests/docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, that's right, I forgot about that. I guess it's okay if I disable collapsing for the category "HTML items tests".

@netlify
Copy link

netlify bot commented Feb 6, 2022

✔️ [V2]

🔨 Explore the source changes: 1f26b6b

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

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

@github-actions
Copy link

github-actions bot commented Feb 6, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 94
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

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

@github-actions
Copy link

github-actions bot commented Feb 6, 2022

Size Change: +2.75 kB (0%)

Total Size: 774 kB

Filename Size Change
website/.docusaurus/globalData.json 48 kB +786 B (+2%)
website/build/assets/css/styles.********.css 105 kB -72 B (0%)
website/build/assets/js/main.********.js 584 kB +2.04 kB (0%)
ℹ️ View Unchanged
Filename Size
website/build/index.html 36.7 kB

compressed-size-action

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.

welcome back 😄

LGTM, just move dogfood to appropriate section so that we can review more edge-cases

(I'm particularly interested to inspect the site with JS disable in multiple scenarios because this is why we added hrefWithSSRFallback)

@@ -18,6 +18,7 @@ const sidebars = {
type: 'generated-index',
},
collapsed: false,
collapsible: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have another section to dogfood now, maybe you can move these tests there?

(one with/without link and with/without collapsible?)

https://deploy-preview-6622--docusaurus-2.netlify.app/tests/docs

@lex111
Copy link
Contributor Author

lex111 commented Feb 9, 2022

Thanks! I still want to implement some of my ideas in the near future.

website/sidebars.js Outdated Show resolved Hide resolved
@Josh-Cena
Copy link
Collaborator

I'm not sure, we should probably test the following cases:

  • Non-collapsible, with category index
  • Collapsible, with category index
  • Non-collapsible, without category index
  • Collapsible, without category index

Currently, the first one does not have a test case. Worth adding one under the Tests/Category Links category

@lex111
Copy link
Contributor Author

lex111 commented Feb 10, 2022

It's make sense to get a full picture, although it is not necessary for this fix.
Just added a new category right after main category.

@Josh-Cena
Copy link
Collaborator

The effect looks good to me!

@Josh-Cena Josh-Cena merged commit cfef475 into main Feb 11, 2022
@Josh-Cena Josh-Cena deleted the lex111/docsidebar-clean-css branch February 11, 2022 05:11
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.

None yet

4 participants