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: improve theme specificity #506

Merged
merged 34 commits into from
Apr 1, 2021
Merged

Conversation

sarahdayan
Copy link
Member

@sarahdayan sarahdayan commented Mar 29, 2021

This PR takes care of the theme's specificity issues, and restructures it for clarity.

Specificity

Here's the before/after of the compiled theme's specificity:

Before

112172753-453ff580-8bf5-11eb-9537-e38421ff54a3

After

Capture d’écran 2021-03-29 à 12 49 52

There are still specificity spikes but they're lower, more even (less "levels" of specificity) and there are many more flat segments.

The remaining specificity spikes are caused by:

  • Pseudo-classes (e.g., .aa-Form:focus-within, .aa-ClearButton:hover). These are unavoidable.
  • Qualified selectors (e.g., body.dark, .aa-ClearButton[hidden]). These are mostly unavoidable, or a refactor wouldn't bring much value.
  • Nested elements (e.g., .aa-Panel button, .aa-ItemContent mark). These can be avoided by adding a class on the element instead. At this stage, I don't think it's necessary but we can keep it in mind for later improvements.
  • Nested classes (e.g., .aa-ItemContent--dual .aa-ItemContentTitle, .aa-DetachedContainer .aa-Panel). This makes sense in the case of modifiers, or context classes like .aa-DetachedContainer, and we should make sure to limit this practice to these two use cases.
  • Compounds of the above practices (e.g., .aa-ItemActionButton:hover svg, .aa-Item[aria-selected=true] .aa-ItemActionButton).

Specificity is reduced by bringing most of the selectors that don't need any specificity at the root of the style sheet (using @at-root). Sometimes this isn't possible, because we need context nesting (e.g., .aa-DetachedContainer .aa-SourceHeader). In this case, I manually brought all selectors at the same level (the first) because there's no Sass directive that lets you "dedent" levels of specificity.

Structure

With specificity issues fixed, I reworked the nested structures so that they make more sense for users when they read it, and it's easier to iterate on.

A typical unit looks like this:

.Element {
  :pseudo-classes, @media queries {}
  &[attribute-qualifiers], &.class-qualifiers {}
  :pseudo-elements, .ChildrenElementsThatNeedToBeScoped, nested-elements {}
  @at-root .ChildrenElementsThatDontNeedToBeScoped {}
}

The idea is to have, in order:

  1. All selector-related styles (direct styles, pseudo-classes styles)
  2. All styles that directly qualify the selector (.element[hidden], .element.dark) and their descendants
  3. All children that must be stylistically scoped (which styles depend on the parent)
  4. All children that don't need to be stylistically scoped, but are nested for better DX

This structure allows us to have all element-related styles together (e.g., avoiding lone pseudo-classes and media queries at the bottom, without knowing who they belong to) and use specificity without clashing with source order.

Found issues

I've noticed a couple of issues in the styles which would require to "break" the current API if we tackle them.

.aa-ItemIcon vs. .aa-Visual

From the markup in the playground, it seems they're meant to be used together (.aa-Visual relies on styles from .aa-ItemIcon). If this is the case, then I'd suggest going with a modifier class instead of .aa-Visual, to convey that importance.

For example:

@at-root .aa-ItemIcon {
    // common styles and children
  }
  @at-root .aa-ItemIcon--symbol {
    // styles and children for icon/SVG usage
  }
  @at-root .aa-ItemIcon--image {
    // styles and children for images
  }
}

.aa-TouchOnly

From the styles, it seems that .aa-TouchOnly is supposed to hide elements that make no sense on mobile (we use it on the "Select" button and it has display: none). If this is the case, then the name is contradictory, it should be .aa-DesktopOnly.

.aa-TouchOnly and .aa-ActiveOnly

These are utility classes, yet they're nested under .aa-Item (but brought at root). Are they meant to be reused elsewhere? If yes, they shouldn't be nested, but kept at the end and made important to always defeat specificity. Otherwise, if they're only meant to be used with .aa-ItemActionButton, they should be modifiers.

.aa-ItemContentSubtitleInline

This class is used in parallel of .aa-ItemContentSubtitle as if they were two different concerns, but the former is actually a modifier of the latter. If we make it a proper .aa-ItemContentSubtitle--inline modifier (which must then be used together with .aa-ItemContentSubtitle), then can remove the specificity of .aa-ItemContentSubtitle .aa-ItemContentSubtitleIcon and bring .aa-ItemContentSubtitleIcon at root, then stylistically scope it under .aa-ItemContentSubtitle--inline.

Review highlights

I've tested the changes in the playground and as far as I can tell, nothing broke.

Since you know the different states better than me, feel free to do a pass to ensure nothing moved.

What's next

  • Move from placeholders to mixins and compare the impact on file size
  • Discuss and make a decision on the found issues

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 29, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c2ee4cf:

Sandbox Source
@algolia/autocomplete-example-github-repositories-custom-plugin Configuration
@algolia/autocomplete-example-playground Configuration
@algolia/autocomplete-example-query-suggestion-with-categories Configuration
@algolia/autocomplete-example-query-suggestions-with-hits Configuration
@algolia/autocomplete-example-query-suggestions-with-inline-categories Configuration
@algolia/autocomplete-example-query-suggestions-with-recent-searches Configuration
@algolia/autocomplete-example-query-suggestions Configuration
@algolia/autocomplete-example-react-renderer Configuration
@algolia/autocomplete-example-recently-viewed-items Configuration

@sarahdayan sarahdayan marked this pull request as ready for review March 29, 2021 14:39
@sarahdayan sarahdayan requested a review from Shipow March 29, 2021 14:39
@sarahdayan
Copy link
Member Author

I'll resolve conflicts once #497 is merged.

@francoischalifour
Copy link
Member

Thanks for the exhaustive review!

.aa-ItemIcon vs. .aa-ItemVisual

I think they were used together for simplicity in the sandbox at first but IMO it'd make sense to separate the two classes (and to share common properties in the stylesheet).

.aa-TouchOnly

Good catch. And perhaps we should have a utility class for desktop too.

.aa-TouchOnly and .aa-ActiveOnly

.aa-ActiveOnly should be scoped under .aa-Item because it reflects the active state of the item.

However, there's no reason to scope .aa-TouchOnly under .aa-Item.

.aa-ItemContentSubtitleInline

This and .aa-ItemContentSubtitle are two different use cases depending on your item layout, so I'm not sure if the inline is really a modifier, but rather another styling strategy. I'll let you judge on that one though.

Base automatically changed from feat/theme-patch to next April 1, 2021 10:09
@sarahdayan sarahdayan merged commit e81c31f into next Apr 1, 2021
@sarahdayan sarahdayan deleted the feat/theme-specificity branch April 1, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants