feat(content-docs): allow setting class name for category link by Josh-Cena · Pull Request #6249 · facebook/docusaurus

Josh-Cena · 2022-01-02T10:51:30Z

Motivation

Close #6232.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Added a dogfood example

netlify · 2022-01-02T10:59:29Z

✔️ [V2]

🔨 Explore the source changes: 8703ac1

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

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

github-actions · 2022-01-02T10:59:53Z

⚡️ Lighthouse report for the changes in this PR:

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

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

github-actions · 2022-01-02T11:06:08Z

Size Change: +2.23 kB (0%)

Total Size: 672 kB

Filename	Size	Change
`website/.docusaurus/globalData.json`	40.7 kB	+588 B (+1%)
`website/build/assets/js/main.********.js`	499 kB	+1.64 kB (0%)

ℹ️ View Unchanged

Filename	Size
`website/build/assets/css/styles.********.css`	102 kB
`website/build/index.html`	29.6 kB

_{compressed-size-action}

slorber

Thanks 👍

We can probably simplify this PR a bit: just use category.className instead of category.link.className

Note that there's a very similar issue: #6254
Probably simple enough to be solved in this PR.

slorber · 2022-01-05T17:32:53Z

packages/docusaurus-plugin-content-docs/src/sidebars/generator.ts

            type: 'doc',
            id: categoryLinkedDocId, // We "remap" a potentially "local id" to a "qualified id"
+            className:
+              categoryMetadata?.link?.className ??


we already have categoryMetadata?.className and it solves user reported use-case, why introduce a new categoryMetadata?.link?.className here?

You could do this instead:

category.className = categoryMetadata?.className ?? getDoc(link.id).frontMatter.sidebar_class_name

slorber · 2022-01-05T17:33:43Z

packages/docusaurus-plugin-content-docs/src/sidebars/types.ts

+export type SidebarItemCategoryLinkDoc = {
+  type: 'doc';
+  id: string;
+  className?: string;


Is this new attribute really necessary? What's the advantage of having it instead of reusing the existing category className? Will this be applied to something else that is not the category?

slorber · 2022-01-05T17:36:32Z

packages/docusaurus-theme-classic/src/theme/DocSidebarItem/index.tsx

+          ThemeClassNames.docs.docSidebarItemCategoryLink,
+          ThemeClassNames.docs.docSidebarItemCategoryLinkLevel(level),
+          'menu__list-item-collapsible',
+          linkClassName,


The user wanted the className to be applied to the <li> item, so may not exactly solve the user problem

slorber · 2022-01-05T17:41:11Z

packages/docusaurus-theme-classic/src/theme/DocSidebarItem/index.tsx

+      <div
+        className={clsx(
+          ThemeClassNames.docs.docSidebarItemCategoryLink,
+          ThemeClassNames.docs.docSidebarItemCategoryLinkLevel(level),


do we really need those extra classes stable? What's the purpose?

I know we want stable selectors but IMHO on parent it's good enough, it's quite easy to target the first child div once we can target the parent reliably

slorber · 2022-01-05T17:45:40Z

packages/docusaurus-plugin-content-docs/src/props.ts

-      frontMatter: {sidebar_label: sidebarLabel},
+      frontMatter: {
+        sidebar_label: sidebarLabel,
+        sidebar_class_name: sidebarClassName,


sidebar_class_name intuitively implies to me that it will apply as well to doc items in the sidebar, and not just category

Maybe we should make it more explicit?

sidebar_category_class_name ? Or apply to doc and categories both?

Josh-Cena · 2022-01-06T04:38:39Z

So we have a category class name already, but this one targets the entire category dropdown rather than the link itself. (I designed it like that initially because at that time there wasn't the extra div around the link, and targeting the entire dropdown allows people to customize margins, etc.) The changes in the DOM structure after the category index pages have caused troubles on the TS-eslint website. Adding another class name helps ensure a stable semantic selector, since otherwise if you want to style the category link, you have do .theme-doc-sidebar-item-category > div > a which looks like subjecting to another breaking change in the future. A new link class name makes it more consistent.

slorber · 2022-01-06T11:31:53Z

Adding another class name helps ensure a stable semantic selector,

Agree that we could add another semantic selector here, but that seems like a different subject.

Does a new .theme-doc-sidebar-item-category-group for the parent li makes sense to you? (breaking change, not sure how to find a better semantic className for the child div 🤷‍♂️ )

When using sidebar_class_name: green the user expect this class to be applied to the "li" item, not the child div.

This frontmatter should allow customizing a whole category li item, including subitems.

If it's only applied to the child div, it's less powerful and it's definitively not what the original issue asked for.

We should be able to write something like:

.theme-doc-sidebar-item-category-group.green > .theme-doc-sidebar-item-category > a {
  color: green;
}

But also

.theme-doc-sidebar-item-category-group.green {
  border-left: solid green;
}

Also I think the current desing/API is mixing 2 things: the category linking feature, and the sidebar category label link. Those share some "link" terminology but are not strictly related. And it's even weird that you have a link.className and don't even apply this className to the <a> element in the end 🤪

Josh-Cena · 2022-01-06T11:35:01Z

Wait, eh? I didn't realize they wanted it on the <li> element...

In that case, maybe that issue is actually related to #6254?

Can we guarantee DOM stability in the future? In that case we may just recommend .theme-doc-sidebar-item-category > div > a and make all category class names target the entire dropdown rather than the category link.

I implemented the link class name as targeting <li> rather than <a> because it's always easier to select a children (particularly when there can only be one) than select a parent. Targeting a block element also sounds like a useful thing for sizing

slorber · 2022-01-06T12:02:39Z

Wait, eh? I didn't realize they wanted it on the
element...
In that case, maybe that issue is actually related to Autogenerated category with doc link: use doc frontmatter for category metadata #6254?

Yes it's related, see my previous comment :)

We should apply sidebar_label and sidebar_class_name as fallbacks for _category_.json

Can we guarantee DOM stability in the future? In that case we may just recommend .theme-doc-sidebar-item-category > div > a and make all category class names target the entire dropdown rather than the category link.

I don't really like to guarantee DOM structure stability, so there's probably still value in creating a sub-selector for the category "row" (excluding child), but not sure how to find good names 🤷‍♂️ However I don't think this selector is likely to be refactored so it should be pretty stable anyway.

In general, I think our public API surface is the theme components interface and stable CSS classNames. All other CSS and HTML structures should be considered as implementation details, subject to change.

slorber · 2022-01-06T12:04:22Z

I implemented the link class name as targeting
rather than

You mean <div>?

because it's always easier to select a children (particularly when there can only be one) than select a parent.

Same reason for applying the className on <li> instead: you can still select the first div quite easily, but the opposite is not true

Josh-Cena · 2022-01-06T12:12:56Z

You mean <div>?

No, I mean <li> of each doc link in the dropdown, before we had category indexes

Same reason for applying the className on <li> instead: you can still select the first div quite easily, but the opposite is not true

Yes, absolutely no objection to applying class name to <li> (which we do already). Just that "you can still select the first div" is defeated if we change DOM structure in the future.

So the resolution is:

Let the sidebar_class_name of index pages target the entire category (which will be consistent with other metadata once we allow using category index page metadata to set category metadata)
Give a new stable class name to category links, without letting users define a custom one because the equivalent can be achieved by using .category > div > a?

slorber · 2022-01-06T15:51:35Z

yes 👍

But should the new stable selector apply to the div or the a? What could be a good name for it?

Josh-Cena · 2022-03-06T10:58:03Z

Going to close this as it's not in the right direction

feat(content-docs): allow setting class name for category link

db872ae

Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Jan 2, 2022

Josh-Cena requested review from lex111 and slorber as code owners January 2, 2022 10:51

facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 2, 2022

Josh-Cena added 4 commits January 2, 2022 19:47

Test all cases

67d7724

docs

06d413f

stable class name

c29651e

Update index.md

8703ac1

slorber requested changes Jan 5, 2022

View reviewed changes

Josh-Cena closed this Mar 6, 2022

Josh-Cena deleted the jc/category-index-class-name branch March 6, 2022 10:58

Josh-Cena mentioned this pull request Mar 6, 2022

feat(content-docs): autogenerate category with linked doc metadata as fallback #6859

Merged

Uh oh!

Comments

Conversation

Josh-Cena commented Jan 2, 2022

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Uh oh!

netlify bot commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slorber left a comment

Choose a reason for hiding this comment

Uh oh!

slorber Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

slorber Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

slorber Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

slorber Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

slorber Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

Josh-Cena commented Jan 6, 2022

Uh oh!

slorber commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Josh-Cena commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slorber commented Jan 6, 2022

Uh oh!

slorber commented Jan 6, 2022

Uh oh!

Josh-Cena commented Jan 6, 2022

Uh oh!

slorber commented Jan 6, 2022

Uh oh!

Josh-Cena commented Mar 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlify bot commented Jan 2, 2022 •

edited

Loading

github-actions bot commented Jan 2, 2022 •

edited

Loading

github-actions bot commented Jan 2, 2022 •

edited

Loading

slorber commented Jan 6, 2022 •

edited

Loading

Josh-Cena commented Jan 6, 2022 •

edited

Loading