refactor(theme-classic): consistently add span wrapper for layout links

[Josh-Cena]

Motivation

I'm constantly frustrated by the inconsistent DOM between internal and external links, because that means if I want to apply CSS to the text, I have to target both a and a > span.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Should have no layout changes

[netlify]

✔️ [V2]

🔨 Explore the source changes: 9b10c23

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

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

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

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

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

[github-actions]

Size Change: +310 B (0%)

Total Size: 791 kB

Filename	Size	Change
website/build/index.html	38.7 kB	+312 B (+1%)

ℹ️ View Unchanged

Filename	Size	Change
website/.docusaurus/globalData.json	49.9 kB	0 B
website/build/assets/css/styles.********.css	105 kB	0 B
website/build/assets/js/main.********.js	597 kB	-2 B (0%)

_{compressed-size-action}

[slorber]

I agree but at the same time it does not lead to the exact same result 😅

• https://deploy-preview-6846--docusaurus-2.netlify.app/tests/docs
• https://docusaurus.io/tests/docs

The footer looks fine, and we don't have preview tests for navbar link item.

Maybe it would be better to add a span consistently to the sidebar item?

[Josh-Cena]

Oh, huh, I rather like the current result 😄 I think we once right-aligned the icons but later made them flushed left? Can't remember why though

[slorber]

For consistency with navbar and footer and UI retro compatibility (this PR is not about design) I'd rather keep the original design for now

Open to discussing design changes but IMHO it's not really better.

Maybe we can create a poll on discord or something

[Josh-Cena]

Yeah, not saying I want to introduce that difference in this PR, just that I honestly think it makes sense.

[slorber]

seems to work fine thanks 👍

[lex111]

This was actually done intentionally, personally I've always been against extra elements that aren't needed at all. Why is consistency so important in this case? Its lack didn't affect on customization, at least there were no complaints from users, right? Currently I see just a bunch of extra wrapper elements -- it complicates final DOM and increases page size (a bit, but still...).

because that means if I want to apply CSS to the text

You just need to target on link element, or use navbar__link or footer__link-item class.

[Josh-Cena]

I was the one that complained, because I do lots of customizations on my own sites😅 In order to target the text node my CSS has to reach into that span like theme-doc-sidebar-item-link > a > span.

You just need to target on link element, or use navbar__link or footer__link-item class.

These Infima class names are discouraged to be targeted in our docs because they are implementation details. I also don't feel like it's worth adding another stable class name that's just the direct child of the current theme-doc-sidebar-item-link class name.

I understand the concern of bundle size—I also have a page size OCD. We may do another minor breaking change in the near future to remove that span, but that depends on whether we want the external link icons to be right-aligned (behavior without the span tag) or left-aligned (behavior with the span tag).

[lex111]

I'm not sure this is efficient, in the sense that it's a rare use case anyway, and it's rather acceptable to target on Infima classes, given that the custom CSS file is designed for that purpose. Why not just target on <a> element?

I mean, curretly this change affects on all sites, regardless of the fact that it was mostly avoidable, which is not a good thing.

[slorber]

I think in the future it might be worth it to keep this extra spam and add a "stable className" to the label, eventually have a dedicated component just for that kind of sidebar label in our design system

There's a tradeoff to be made:

• Optimizing for HTML output size
• Optimizing for easy and robustness of customizations

Unfortunately, it's hard to have both at the same time.

In general I'd rather optimize for the later, as IMHO nobody ever complained that the site is 5-10% heavier, while many feel the pain of upgrading a highly customized site due to CSS selectors breaking.

When we add extra wrappers and stable classNames (non-infima), we increase HTML size on purpose to increase customization robustness.

[lex111]

Alright, I found a simple solution on how we can get rid of span altogether. I guess I should have originally implemented such a change as an external icon in Infima itself, so that we could just integrate it into Docusaurus later on. I'm stepping on the same rake again, so let's fix it facebookincubator/infima#214