feat(theme-classic): allow stylizing doc paginator arrows

[noomorph]

Motivation

I'd like to stylize the DocPaginator so it looks like this:

The problem is that the arrows » are just inlined into the markup and cannot be hidden via CSS:

Have you read the Contributing Guidelines on pull requests?

Not yet. 😊

Test Plan

No plan so far, but I'd like to check with you if you agree that this change makes sense. I would refrain from swizzling currently, but I cannot.

[netlify]

✔️ [V2]

🔨 Explore the source changes: 13b645f

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

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

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

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

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

[lex111]

I don't really like this kind of decorative CSS classes that don't have any declarations. So perhaps it would be better if we added arrows into pagination links as ::after pseudo-elements?

[Josh-Cena]

In addition to that, this has an implication that it's an Infima class (because of BEM, I guess) which isn't the case. In similar cases, We would either add this to ThemeClassName (bad idea in this case, overkill) or use ::after pseudoelement as @lex111 suggested

[noomorph]

Will do, thanks. As for the test plans - what's the usual routine for this kind of change?

[Josh-Cena]

Will do, thanks. As for the test plans - what's the usual routine for this kind of change?

Just make sure that the page renders correctly. It's a pretty minor change and we don't have a good UI test setup right now

[noomorph]

Okay, so, hopefully, I implemented it the way you recommended.

[lex111]

@noomorph sorry, probably I didn't explain it good. This kind of change needs to be made in the repository of CSS framework that Docusaurus uses internally, I mean this file: https://github.com/facebookincubator/infima/blob/main/packages/core/styles/components/pagination-nav.pcss
There you will also find file demo/index.html, which should be updated to reflect the new changes.
Then I will do a new release, after that the Infima version should be bumped in the Docusaurus codebase -- this is only change that will have to be done in the current PR.

[noomorph]

@lex111, ok, I'll do that tomorrow. So, here, in the pull request, I'll just need to remove those « and », and that's it, right? (Plus, update Infima after the preliminary PR to it, of course).

[slorber]

Not every fan of using ::before/::after, this is a quite implicit API surface, less extensible and prevents us from CSS/HTML refactorings in the future without annoying users that customize their sites. + that custom CSS might not be compatible from one theme to another.

It would probably be a good idea to have a smaller DocPaginatorLink component:

          <Link className="pagination-nav__link" to={previous.permalink}>
            <div className="pagination-nav__sublabel">
              <Translate
                id="theme.docs.paginator.previous"
                description="The label used to navigate to the previous doc">
                Previous
              </Translate>
            </div>
            <div className="pagination-nav__label">
              &laquo; {previous.title}
            </div>
          </Link>

And maybe even have a DocPaginatorLinkArrow

I'm not against using ::before/::after, just think we'd rather recommend users to swizzle smaller components instead of using complex, theme-related CSS selectors

[noomorph]

Ok, so shall I refactor the component and split it into DocPaginatorLink and DocPaginator?

[slorber]

Let's wait a bit for second opinions :)

I plan to write some theme guidelines so that it's clearer when to create sub-components, how to name them etc... that opens the discussion ;)

[Josh-Cena]

Ultimately, IMO, CSS is a more lightweight customization than swizzling whatsoever. These small icons also have better semantics if they are pseudoelements (same goes for the sidebar chevrons / external link icons). If we implement ::before / ::after in Infima and make the arrows strongly coupled to the pagination-nav__label class, how would that hinder any refactorings in Docusaurus?

this is a quite implicit API surface

Most theme components are implicit API surfaces anyways. Users are expected to read the code to understand which one they need to swizzle. CSS is directly visible DevTools, which is a big reason why I personally go to CSS before even considering swizzling. Also, CSS, especially Infima CSS, is more stable as long as we want to offer the same semantics.

that custom CSS might not be compatible from one theme to another.

Do users really want to jump between themes without being prepared to invest significant effort in making them reconcile?

[lex111]

I agree with @Josh-Cena. It's too small a part of the design to make it a separate component. Especially it's hardly a frequently changed thing by users, so I would use pseudo elements to make it customizable.

[slorber]

I believe we can and should do both.
Agree that CSS is a quite simple way to do such customizations, but it's also more likely to break, and does not allow using images or SVG arrows as easily.

CSS, HTML structure and selectors is a quite implicit API surface, and Facebook internal opinions are that we should respect semver better in the future, so we should try to make implicit customization API surface more explicit

Having a dedicated theme component, even for small things, is a more explicit API surface, and a commitment to our users that we aren't going to refactor this component in a minor version and that it is safe to swizzle without expecting annoying side-effects on upgrades.

Most theme components are implicit API surfaces anyways. Users are expected to read the code to understand which one they need to swizzle.

We can't prevent users to write fancy CSS selectors, but at least we can tell them "hey, the html/css structure is not part of our public API surface, things will break"

What matters is how we communicate breaking changes to our users. The idea is that if a user only uses swizzle on components that we consider safe to override, they should be able to upgrade with no effort on minor versions, without having to carefully review every single page for little CSS issues, as it happens currently.

Do users really want to jump between themes without being prepared to invest significant effort in making them reconcile?

We should aim for that goal. Users could switch theme based on an env variable and it should still work if we do things correctly. I'd even go further and discourage the usage of Infima in user pages (eventually providing some shared Docusaurus layout classes), so that a site can more easily change its theme in the future

[Josh-Cena]

Let's make it clear that we are not going to make the CSS changes here (the current PR is a WIP), but per requested above, it will be done in Infima instead. Would that be considerer more stable? Or do you actually envision that users would never target Infima class names at all?

[slorber]

Let's make it clear that we are not going to make the CSS changes here (the current PR is a WIP), but per requested above, it will be done in Infima instead. Would that be considerer more stable?

We can do the Infima change, it's an implementation detail and not something that we should recommend users to target with CSS. But still, some users may find it useful/convenient to use CSS 🤷‍♂️

Or do you actually envision that users would never target Infima class names at all?

We should aim to never recommend users to target Infima classes, understand the most popular customization use-cases, and provide robust alternatives like dedicated components and stable classNames on which we can guarantee minor version retro-compatibility.

But we can't prevent users to listen anyway

---

@noomorph would you use DocPaginatorArrow over a CSS selector if this existed and was an option that we advised?

[noomorph]

@slorber,

@noomorph would you use DocPaginatorArrow over a CSS selector if this existed and was an option that we advised?

well, that's a good question. In fact, if you look at the vanilla Docusaurus with my style override below, you'll see also that I need to move that "arrow" physically out of the label to the top level. Do you think your DocPaginatorArrow suggestion would have helped in my case?

[slorber]

There are 2 arrows in your screenshot 🤪

But it looks like what you want is to swizzle the full card component (DocPaginatorLink?) and customize its internal layout, so neither DocPaginatorArrow nor a CSS selector override is a good fit.

[noomorph]

There are 2 arrows in your screenshot 🤪

Obviously 😄 because I was illustrating my problem for you (regarding the necessity to move the arrow's DOM position).

But it looks like what you want is to swizzle the full card component (DocPaginatorLink?) and customize its internal layout, so neither DocPaginatorArrow nor a CSS selector override is a good fit.

Yeah, I guess that would work better if we already are speaking about swizzling the components.

[noomorph]

Submitted a PR to Infima: facebookincubator/infima#196

[noomorph]

To synchronize this PR with facebookincubator/infima#196, I just have removed the arrow quotes from the code. This way, this PR is just +2,-2 lines of change.

[slorber]

LGTM thanks 👍