Reduce the size of Box.module.css

[joshfarrant]

The CSS for the Box component was previously ~2,800 lines of code. This PR reduces it to 135 lines.

The result of this is a 22% reduction in the size of our main CSS bundle, and an 18% reduction in the size of our main JavaScript bundle.

	Size Before	Size After	Size Change	% Change
main.css	629 KB	497 KB	-132 KB	-22%
index.js	429 KB	351 KB	-78 KB	-18%
total	1058 KB	848 KB	-210 KB	-20%

There is a trade-off in that the rendered HTML markup for Box may now be slightly longer due to the addition of inline CSS variables when compared to the best-case scenario from the previous implementation. When compared to the worst-case scenario, however, it's about half of the size.

<!-- Before (best case) -->
<div class="Box-module__Box-padding--80__fB_np">
</div>

<!-- Before (worst case) -->
<div class="Box-module__Box-narrow-paddingBlockStart--8__lH0Nx Box-module__Box-regular-paddingBlockStart--12__gOq9H Box-module__Box-wide-paddingBlockStart--16__f5Dfj Box-module__Box-narrow-paddingInlineEnd--8__Rt9bb Box-module__Box-regular-paddingInlineEnd--12__X02EG Box-module__Box-wide-paddingInlineEnd--16__K4CHM Box-module__Box-narrow-paddingBlockEnd--8__y1kaa Box-module__Box-regular-paddingBlockEnd--12__uroYu Box-module__Box-wide-paddingBlockEnd--16__yvgLa Box-module__Box-narrow-paddingInlineStart--8__WQHFH Box-module__Box-regular-paddingInlineStart--12__MFP_O Box-module__Box-wide-paddingInlineStart--16__vDqYc Box-module__Box-narrow-marginBlockStart--8__i685k Box-module__Box-regular-marginBlockStart--12__HLits Box-module__Box-wide-marginBlockStart--16__R8o04 Box-module__Box-narrow-marginInlineEnd--8__jhqqu Box-module__Box-regular-marginInlineEnd--12__VJwpo Box-module__Box-wide-marginInlineEnd--16__JaWXe Box-module__Box-narrow-marginBlockEnd--8__kUGhn Box-module__Box-regular-marginBlockEnd--12__kpsJs Box-module__Box-wide-marginBlockEnd--16__LqPgv Box-module__Box-narrow-marginInlineStart--8__Md4jm Box-module__Box-regular-marginInlineStart--12__LVaCT Box-module__Box-wide-marginInlineStart--16__mUIjr">
</div>

<!-- After (fixed) -->
<div class="Box-module__Box__iXAWK" style="--box-npbs: var(--base-size-80); --box-rpbs: var(--base-size-80); --box-wpbs: var(--base-size-80); --box-npbe: var(--base-size-80); --box-rpbe: var(--base-size-80); --box-wpbe: var(--base-size-80); --box-npis: var(--base-size-80); --box-rpis: var(--base-size-80); --box-wpis: var(--base-size-80); --box-npie: var(--base-size-80); --box-rpie: var(--base-size-80); --box-wpie: var(--base-size-80); --box-nmbs: 0; --box-rmbs: 0; --box-wmbs: 0; --box-nmbe: 0; --box-rmbe: 0; --box-wmbe: 0; --box-nmis: 0; --box-rmis: 0; --box-wmis: 0; --box-nmie: 0; --box-rmie: 0; --box-wmie: 0;">
</div>

Short variable names have been chosen to reduce the impact of this. For example, --box-npie refers to a Box component on a Narrow viewport and sets the Padding-Inline-End property.

Backwards compatibility

I'm pretty confident that the underlying logic hasn't changed and the VRT snapshots suggest the same. However please do let me know if you think there's something I've missed.

Tests

The spacing variables have quite a few tests associated with them — 1008 to be exact. The test suite takes advantage of Jest's .each() function to test all of the variables, for all of the spacing props, against all of the available options (none, condensed, 4, 8, etc). It may seem like overkill, but the benefit of this approach is that it gives really clear messages when something breaks.

For example, if a test fails you will see:

when marginBlockStart > equals 8 > --box-rmbs is not being set to var(--base-size-8)

You can see the test in question here

The benefit of this approach over the established pattern of running a for/of loop inside of the test is that each assertion is part of its own test, which isn't the case when using a for/of loop. When an assertion fails you'll see exactly which test fails, as shown above. If we were to use a for/of loop, when an assertion fails the whole test will fail, and we won't get the same level of detail in the test output.

Test performance

Running tests is pretty fast. Running all 1008 tests takes <3.5 seconds on my machine. Looking at recent runs of the CI / Install and run tests Action it looks like the impact of these additional tests is pretty negligible.

The number of tests could be reduced by reducing the number of spacing props we're testing against. Instead of testing against all of our valid base scale values (4, 8, 12, 16, etc) we could just pick one number to test along with our string spacings ('none', 'condensed', 'normal', 'spacious'). This would bring the number of tests down to 240, however it would only reduce the time taken to run the tests by ~10%.

What should reviewers focus on?

• Happy with the implementation?
• Happy with the tests?
• Can you find any visual regressions, or reasons that the underlying logic might have changed?

Contributor checklist:

• All new and existing CI checks pass
• Tests prove that the feature works and covers both happy and unhappy paths
• Any drop in coverage, breaking changes or regressions have been documented above
• New visual snapshots have been generated / updated for any UI changes
• All developer debugging and non-functional logging has been removed
• Related issues have been referenced in the PR description

Reviewer checklist:

• Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
• Check that tests prove the feature works and covers both happy and unhappy paths
• Check that there aren't other open Pull Requests for the same update/change

[changeset-bot]

🦋 Changeset detected

Latest commit: f16cfb0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@primer/react-brand	Minor
@primer/brand-primitives	Minor
@primer/brand-e2e	Minor
@primer/brand-fonts	Minor
@primer/brand-config	Minor
@primer/brand-storybook	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

🟢 No design token changes found

[github-actions]

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences

[rezrah]

The markup for the default Box (without any options) is now quite verbose.

Before:

After:

If values for options aren't specified can we remove the inline styles?

[joshfarrant]

EDIT: See this comment first. I found a way to reduce the markup size, which nullifies the below comment 😄

I touch on that a bit in the PR description:

There is a trade-off in that the rendered HTML markup for Box may now be slightly longer due to the addition of inline CSS variables when compared to the best-case scenario from the previous implementation. When compared to the worst-case scenario, however, it's about half of the size.

<!-- Before (best case) -->
<div class="Box-module__Box-padding--80__fB_np">
</div>

<!-- Before (worst case) -->
<div class="Box-module__Box-narrow-paddingBlockStart--8__lH0Nx Box-module__Box-regular-paddingBlockStart--12__gOq9H Box-module__Box-wide-paddingBlockStart--16__f5Dfj Box-module__Box-narrow-paddingInlineEnd--8__Rt9bb Box-module__Box-regular-paddingInlineEnd--12__X02EG Box-module__Box-wide-paddingInlineEnd--16__K4CHM Box-module__Box-narrow-paddingBlockEnd--8__y1kaa Box-module__Box-regular-paddingBlockEnd--12__uroYu Box-module__Box-wide-paddingBlockEnd--16__yvgLa Box-module__Box-narrow-paddingInlineStart--8__WQHFH Box-module__Box-regular-paddingInlineStart--12__MFP_O Box-module__Box-wide-paddingInlineStart--16__vDqYc Box-module__Box-narrow-marginBlockStart--8__i685k Box-module__Box-regular-marginBlockStart--12__HLits Box-module__Box-wide-marginBlockStart--16__R8o04 Box-module__Box-narrow-marginInlineEnd--8__jhqqu Box-module__Box-regular-marginInlineEnd--12__VJwpo Box-module__Box-wide-marginInlineEnd--16__JaWXe Box-module__Box-narrow-marginBlockEnd--8__kUGhn Box-module__Box-regular-marginBlockEnd--12__kpsJs Box-module__Box-wide-marginBlockEnd--16__LqPgv Box-module__Box-narrow-marginInlineStart--8__Md4jm Box-module__Box-regular-marginInlineStart--12__LVaCT Box-module__Box-wide-marginInlineStart--16__mUIjr">
</div>

<!-- After (fixed) -->
<div class="Box-module__Box__iXAWK" style="--box-npbs: var(--base-size-80); --box-rpbs: var(--base-size-80); --box-wpbs: var(--base-size-80); --box-npbe: var(--base-size-80); --box-rpbe: var(--base-size-80); --box-wpbe: var(--base-size-80); --box-npis: var(--base-size-80); --box-rpis: var(--base-size-80); --box-wpis: var(--base-size-80); --box-npie: var(--base-size-80); --box-rpie: var(--base-size-80); --box-wpie: var(--base-size-80); --box-nmbs: 0; --box-rmbs: 0; --box-wmbs: 0; --box-nmbe: 0; --box-rmbe: 0; --box-wmbe: 0; --box-nmis: 0; --box-rmis: 0; --box-wmis: 0; --box-nmie: 0; --box-rmie: 0; --box-wmie: 0;">
</div>

Short variable names have been chosen to reduce the impact of this. For example, --box-npie refers to a Box component on a Narrow viewport and sets the Padding-Inline-End property.

The reason we need to specify all of the values, even when they're not set, is to ensure that any unused CSS variables are reset to 0. Consider the following example

<Box paddingBlockStart={16}>
  <Box paddingBlockEnd={8}></Box>
</Box>

If we didn't set the CSS variables back to 0 then the inner Box would inherit the paddingBlockStart variables (--box-npbs, --box-rpbs, --box-wpbs) defined on the outer Box, which would then apply padding-block-start to the inner Box.

I believe we'll be able to work around this once we're able to use @property in a few months, since we'll be able to set inherits: false; when defining the property, removing the need for us to zero-out the unused values.

[rezrah]

I'm kind of struggling to decipher what's going on in the markup now 😅. It's also ballooned in size, which I'm wondering whether we can optimize a bit more?

Before:

<div class="Box-module__Box-paddingBlockStart--condensed__mdhHH">...</div>

After:

<div 
  class="Box-module__Box__iXAWK" style="--box-npbs: var(--brand-box-spacing-condensed); --box-rpbs: var(--brand-box-spacing-condensed); --box-wpbs: var(--brand-box-spacing-condensed); --box-npbe: 0; --box-rpbe: 0; --box-wpbe: 0; --box-npis: 0; --box-rpis: 0; --box-wpis: 0; --box-npie: 0; --box-rpie: 0; --box-wpie: 0; --box-nmbs: 0; --box-rmbs: 0; --box-wmbs: 0; --box-nmbe: 0; --box-rmbe: 0; --box-wmbe: 0; --box-nmis: 0; --box-rmis: 0; --box-wmis: 0; --box-nmie: 0; --box-rmie: 0; --box-wmie: 0;">...</div>

Taken from this story: https://primer-58e37265a8-26139705.drafts.github.io/brand/storybook/?path=/story/components-box-features--directional-padding-block-start

• Before we could easily infer the styles being applied from the css class
  • Now it assumes you know what the shorthand var names mean. While it's output, readable names help us (users and maintainers) debug faster. Wondering if there's a middle ground?
• Are we introducing specificity issues moving from classes to inline styles for core logic?

[joshfarrant]

See my above comment on the markup size.

Happy to find a middle ground in terms of naming, but unfortunately longer names will come at the cost of markup size. I tried to be quite clear about how the variable names are created here, but I'd be happy to hear any suggestions on how to make things a bit clearer.

Possibly a controversial take, but I'd say that we shouldn't be too concerned about our outputted class names as any consumer could overwrite the class names to be anything they like when bundling their app. As an aside, I'm actually surprised that doesn't happen more, since Box-module__Box-paddingBlockStart--condensed__mdhHH could be replaced with a random 3 character class name during when it's bundled into the consumers code, which would save quite a lot over the wire.

On the specificity question, we're not really using inline styles as we're not directly applying styles to the element using the style prop. We're still applying styles in the CSS just like we do with any other component, so specificity and ability to override is unaffected.

[joshfarrant]

Thanks for the review @rezrah, appreciate you taking the time to dig into it.

One thing I forgot to note in the PR description is that there's already precedent for a similar pattern within the Section component.

https://github.com/primer/brand/blob/main/packages/react/src/Section/Section.tsx#L123-L133

https://github.com/primer/brand/blob/main/packages/react/src/Section/Section.module.css#L8-L15

<section
  style="--brand-Section-background-image-position: 50%; --brand-Section-background-image-size: cover;"
/>

It's a bit less pronounced here as we (hopefully 😅) don't have to worry about consumers nesting Sections within Sections, and so we don't have to reset unused values as I do with Box. We're also likely to only have a few Section components on a page — compared to a lot of Box components — so there's less pressure to reduce the size of the CSS variable names.

[joshfarrant]

@rezrah Just to note, I dug into this a bit more and managed to find a better solution to resetting the CSS variables, and it turns out we don't need @property at all as I previously thought 🎉

Take a look at 6f6574d, it's actually more straightforward than I initially thought.

This change removes the need for all of the --box-npie: 0; --box-rpie: 0; resets and reduces the size of the markup. If you think the shortened variable names are an issue then this change opens the door to us replacing them with more verbose strings, eg:

- --box-npie
+ --box-narrow-padding-inline-end