refactor(theme-classic): clean up CSS of doc cards #6950

lex111 · 2022-03-21T10:56:13Z

Motivation

Removing unnecessary CSS code and trying to use transform for hover state, because changing background only in dark mode breaks the consistency of the current visual design.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview.

Related PRs

lex111 · 2022-03-21T10:57:01Z

And if I understand correctly, doc cards can not be without a link? So is that why this code can be removed also?

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

Line 38 in 0e13356

<div className={className}>{children}</div>

netlify · 2022-03-21T11:04:30Z

✅ [V2]

Name	Link
🔨 Latest commit	`7d20f5f`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/623da0ec6b4a1900089132d7
😎 Deploy Preview	https://deploy-preview-6950--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-03-21T11:04:56Z

⚡️ Lighthouse report for the changes in this PR:

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

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

github-actions · 2022-03-21T11:05:34Z

Size Change: -265 B (0%)

Total Size: 805 kB

Filename	Size	Change
`website/build/assets/css/styles.********.css`	105 kB	+475 B (0%)
`website/build/assets/js/main.********.js`	612 kB	-233 B (0%)
`website/build/index.html`	38.3 kB	-507 B (-1%)

ℹ️ View Unchanged

Filename	Size
`website/.docusaurus/globalData.json`	49.9 kB

_{compressed-size-action}

Josh-Cena · 2022-03-21T12:18:11Z

doc cards can not be without a link?

It could actually happen, when findFirstCategoryLink returns undefined.

The only case in reality that I can think of right now is when a category contains only pure HTML items.

lex111 · 2022-03-21T14:14:12Z

How about hiding such categories without a link? Why would they show them in that case?

Josh-Cena · 2022-03-21T14:41:56Z

Yeah, it makes sense to hide them IMO🤷‍♂️Can't think of any other cases where a card doesn't have an href.

…d-css-ref

Josh-Cena · 2022-03-22T05:03:55Z

I like the border color change! However, I also find some value in the card slightly illuminating in dark mode...

lex111 · 2022-03-22T05:22:19Z

Alright, replaced with --ifm-color-primary.

Josh-Cena · 2022-03-22T05:37:46Z

Uh, miswording there. I meant that the background should probably be lightened a little bit like before when hovering, in addition to the highlighted border. But the current one looks fine as well.

lex111 · 2022-03-22T07:58:06Z

Background then needs to be changed on hover state for all color modes, but currently it is required only for dark one. However, this breaks the design consistency, so I decided to highlight border instead for all modes.

Josh-Cena · 2022-03-22T07:59:30Z

I think in light mode it thickens the box shadow instead. Since box shadow is usually substituted with lighter foreground in dark mode to signal elevation.

lex111 · 2022-03-22T08:05:01Z

Yes, so border seems like a reasonable alternative to background in that case.

Josh-Cena · 2022-03-22T08:05:44Z

Yeah, still looks good to me. It might also be good for accessibility since the contrast seems more significant

slorber

thanks 👍

Requesting some minor changes

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

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

lex111 · 2022-03-24T14:43:59Z

Done, I also made a few minor semantic improvements.

slorber · 2022-03-24T16:08:20Z

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

@@ -81,7 +81,7 @@ function CardCategory({item}: {item: PropSidebarItemCategory}): JSX.Element {
        {count: item.items.length},
      )}
    />
-  );
+  ) : null;


same as before, I'd like any comp in DocCard to never render null as it's the responsibility of the list to filter or not items upfront

That's because the findFirstCategoryLink function can return undefined. But since we have a validation rule that prevents empty items in categories, we can assume that there will always be a link.

slorber · 2022-03-24T16:09:52Z

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

@@ -15,11 +15,20 @@ export default function DocCardList({
 }: {
  items: PropSidebarItem[];
 }): JSX.Element {
+  const filteredItems = items.filter((item) => {


not sure this test is so great as it potentially not filter categories that have no link

the rule for filtering (not rendering) an item should be clear and in a single place (here)

Don't really understand, can you give concrete use case for testing?

slorber · 2022-03-25T09:47:26Z

Understand better now, did some minor edits to make things clearer 👍 LGTM

Josh-Cena · 2022-03-25T09:48:09Z

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

+function filterItems(items: PropSidebarItem[]): PropSidebarItem[] {
+  return items.filter((item) => {
+    if (item.type === 'category') {
+      return !!findFirstCategoryLink(item);


That seems more expensive than before because we traverse the entire subtree twice. Is it worthwhile?

Oh, otherwise we render an empty article container😅 Well, fair then. A little nasty

That seems more expensive than before

Agree but at the same time it's unlikely that the user build complex sidebar trees without any link, in most cases the link is found fast

Yeah, makes sense 👍

Josh-Cena · 2022-03-25T09:57:41Z

The web is down these days, CI just doesn't kick😕

refactor: cleanup CSS of doc cards

41dd19f

lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Mar 21, 2022

lex111 requested review from slorber and Josh-Cena as code owners March 21, 2022 10:56

facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 21, 2022

lex111 added 4 commits March 22, 2022 07:36

Hide non-clickable doc cards

5439654

Better styling

ee3184a

Merge branch 'main' of github.com:facebook/docusaurus into lex111/car…

a33449a

…d-css-ref

Remove excess transform

73bb51b

Use normal primary color for border

6aab074

Josh-Cena changed the title ~~refactor: cleanup CSS of doc cards~~ refactor(theme-classic): clean up CSS of doc cards Mar 22, 2022

slorber requested changes Mar 24, 2022

View reviewed changes

packages/docusaurus-theme-classic/src/theme/DocCard/index.tsx Outdated Show resolved Hide resolved

packages/docusaurus-theme-classic/src/theme/DocCardList/index.tsx Show resolved Hide resolved

lex111 added 3 commits March 24, 2022 14:59

Proper filtering items

30a3478

Minor fixes

f1362d9

Better semantic markup

ba363c3

slorber requested changes Mar 24, 2022

View reviewed changes

minor changes

6e83bab

Josh-Cena reviewed Mar 25, 2022

View reviewed changes

Josh-Cena and others added 2 commits March 25, 2022 17:49

Update index.tsx

6c319d4

lint

7588984

Empty-Commit

7d20f5f

slorber merged commit 78ecff9 into main Mar 25, 2022

slorber deleted the lex111/card-css-ref branch March 25, 2022 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(theme-classic): clean up CSS of doc cards #6950

refactor(theme-classic): clean up CSS of doc cards #6950

lex111 commented Mar 21, 2022

lex111 commented Mar 21, 2022

netlify bot commented Mar 21, 2022 •

edited

Loading

github-actions bot commented Mar 21, 2022 •

edited

Loading

github-actions bot commented Mar 21, 2022 •

edited

Loading

Josh-Cena commented Mar 21, 2022

lex111 commented Mar 21, 2022

Josh-Cena commented Mar 21, 2022

Josh-Cena commented Mar 22, 2022

lex111 commented Mar 22, 2022

Josh-Cena commented Mar 22, 2022

lex111 commented Mar 22, 2022

Josh-Cena commented Mar 22, 2022

lex111 commented Mar 22, 2022

Josh-Cena commented Mar 22, 2022 •

edited

Loading

slorber left a comment

lex111 commented Mar 24, 2022

slorber Mar 24, 2022

lex111 Mar 24, 2022

slorber Mar 24, 2022

lex111 Mar 24, 2022

slorber commented Mar 25, 2022

Josh-Cena Mar 25, 2022

Josh-Cena Mar 25, 2022

slorber Mar 25, 2022

Josh-Cena Mar 25, 2022

Josh-Cena commented Mar 25, 2022

refactor(theme-classic): clean up CSS of doc cards #6950

refactor(theme-classic): clean up CSS of doc cards #6950

Conversation

lex111 commented Mar 21, 2022

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

lex111 commented Mar 21, 2022

netlify bot commented Mar 21, 2022 • edited Loading

✅ [V2]

github-actions bot commented Mar 21, 2022 • edited Loading

github-actions bot commented Mar 21, 2022 • edited Loading

Josh-Cena commented Mar 21, 2022

lex111 commented Mar 21, 2022

Josh-Cena commented Mar 21, 2022

Josh-Cena commented Mar 22, 2022

lex111 commented Mar 22, 2022

Josh-Cena commented Mar 22, 2022

lex111 commented Mar 22, 2022

Josh-Cena commented Mar 22, 2022

lex111 commented Mar 22, 2022

Josh-Cena commented Mar 22, 2022 • edited Loading

slorber left a comment

Choose a reason for hiding this comment

lex111 commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber commented Mar 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena commented Mar 25, 2022

netlify bot commented Mar 21, 2022 •

edited

Loading

github-actions bot commented Mar 21, 2022 •

edited

Loading

github-actions bot commented Mar 21, 2022 •

edited

Loading

Josh-Cena commented Mar 22, 2022 •

edited

Loading