Add support for per-component stylesheets by aduth · Pull Request #8375 · 18F/identity-idp

aduth · 2023-05-10T20:50:05Z

🛠 Summary of changes

Supports components implementing stylesheet "sidecar" assets, with a similar developer experience to how it functions for JavaScript/TypeScript files.

This is a continuation of the idea originally proposed in #6168, which is now more supportable with USWDS v3's new component-specific imports.

Why?

Developer experience consistency, to support accompanying styles for a component in the same way we already support accompanying scripts
Improved performance, so that users aren't forced to download style data which isn't relevant to the current page
Clearer dependency tracking and separation of concerns, so that there's a closer and more direct connection between styles and the application code that uses it

Results:

This is a minimal proof-of-concept, so the impact is not expected to be drastic.

NODE_ENV=production yarn build:css
brotli-size app/assets/builds/application.css

Before: 29.8kb
After: 29.6kb

📜 Testing Plan

Repeat Testing Plan from #8307. There should be no visual impact of these changes.

aduth · 2023-05-10T20:51:09Z

app/components/clipboard_button_component.css.scss

I'd like to find a better way to do this, since having some files with @use 'uswds-core' as *; and others with @use 'required' as *; is going to cause confusion, but Sass compilation will error when trying to load both uswds-core and required as separate directives.

b8b4f67 is a slight improvement at least as far as restoring @use 'uswds-core' as *; as the conventional way to bring USWDS functions in scope.

It might be more streamlined to go the opposite direction, creating a separate "core" entrypoint that we maintain (basically what required is), and change every stylesheet to use that instead. Then, we still have consistency, while only needing one @use 'core' as *;, vs. separate @forward 'required'; @use 'uswds-core' as *;. The downside is this moves away from how someone might expect to use USWDS. It could be interesting to try to add another layer of "interception" like what LGDS does, so that @use 'uswds-core' as *; is actually loading a stylesheet internal to the IdP.

It could be interesting to try to add another layer of "interception" like what LGDS does, so that @use 'uswds-core' as *; is actually loading a stylesheet internal to the IdP.

Gave this a try in 316cfd431. The hard part was figuring out how to load the "original" uswds-core, but I managed it by adding node_modules as a separate load path and referencing the @18f/identity-design-system path from there.

This might create some possible performance impact depending if more load paths causes a lot of filesystem noise, since we only need to resolve node_modules in this one location, but now it'll be the first thing checked for every @forward, @use, and @import.

This might create some possible performance impact depending if more load paths causes a lot of filesystem noise, since we only need to resolve node_modules in this one location, but now it'll be the first thing checked for every @forward, @use, and @import.

Apparently I forgot that node_modules is a load path by default 😅

identity-idp/app/javascript/packages/build-sass/index.js

Line 34 in c58e368

loadPaths: ['node_modules', ...loadPaths],

zachmargolis · 2023-05-10T21:42:35Z

Do we have a component in mind we wanted to demo this with?

aduth · 2023-05-11T12:38:57Z

Do we have a component in mind we wanted to demo this with?

The changes are already updating ClipboardButtonComponent to use this convention. I'd imagine it could apply generally to most of our components, especially ones which map one-to-one with a design system component, or ones with a few component-specific styles. I think it'll be a little trickier at the moment for components which are also implemented in React, but that might be something we could sort out in the future.

Some other good candidates:

MemorableDateComponent (design system component)
CaptchaSubmitButtonComponent (currently _recaptcha-badge.scss)
ClickObserverComponent (currently _click-observer.scss)
LanguagePickerComponent (currently _language-picker.scss)
ModalComponent (currently _modal.scss)
OneTimeCodeInputComponent (currently _one-time-code-input.scss)
PasswordToggleComponent (currently _password-toggle.scss)
PhoneInputComponent (currently _phone-input.scss)

Ones which would be good to do, but blocked by React usage:

AlertComponent
AccordionComponent
BlockLinkComponent
IconComponent
MemorableDateComponent
PageHeadingComponent (currently _page-heading.scss)
SpinnerButtonComponent
StepIndicatorComponent
TroubleshootingOptionsComponent

I think this would also give us a pattern to create new component semantics around what are currently implemented as ad-hoc CSS classes, e.g.:

_code.scss ➡️ CodeSnippetComponent
_hr.scss ➡️ SeparatorComponent
_password.scss ➡️ PasswordStrengthComponent
_personal-key.scss ➡️ PersonalKeyComponent
_profile-section.scss ➡️ ProfileSectionComponent

aduth · 2023-05-11T12:39:20Z

It might be worth considering if there's more harm than good by splitting out e.g. 3 lines of styles to a separate CSS file, but I'm inclined to say "yes" all things considered, as long as the overhead to implement is minimal (it's actually less work in this proposed approach than in the status quo).

zachmargolis · 2023-05-11T13:42:08Z

The changes are already updating ClipboardButtonComponent to use this convention.

ah whoops yes! the diff to that stylesheet was so small, I didn't register it was a move, makes sense! 👍

zachmargolis · 2023-05-11T13:42:13Z

The changes are already updating ClipboardButtonComponent to use this convention.

ah whoops yes! the diff to that stylesheet was so small, I didn't register it was a move, makes sense! 👍

zachmargolis · 2023-05-11T13:45:27Z

The changes are already updating ClipboardButtonComponent to use this convention.

ah whoops yes! the diff to that stylesheet was so small, I didn't register it was a move, makes sense! 👍

zachmargolis

LGTM

changelog: Internal, Optimization, Reduce size of stylesheet assets for critical path

* Add support for per-component stylesheets changelog: Internal, Optimization, Reduce size of stylesheet assets for critical path * Forward required to use uswds-core conventionally * Override _uswds-core via internal load path * Remove unnecessary load path * Prioritize `--load-path` flag in Sass load path resolution * Add specs for component stylesheets * Add specs for StylesheetHelper * Rename clipboard component stylesheet to simple scss * Render stylesheet_once_tags everywhere application is rendered * Fix component preview for per-component stylesheet

* LG-8349: Move AddressSearch component into own package changelog: Internal, Code Structure, separate AddressSearch UI component as package. * LG-8439: remove old component. * LG-8439: package information. * LG-8439: change log file. * LG-8439: address comments * Update app/javascript/packages/address-search/README.md Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> * Update app/javascript/packages/address-search/index-spec.tsx Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> * LG-8439: extra / not needed. * Add support for per-component stylesheets (#8375) * Add support for per-component stylesheets changelog: Internal, Optimization, Reduce size of stylesheet assets for critical path * Forward required to use uswds-core conventionally * Override _uswds-core via internal load path * Remove unnecessary load path * Prioritize `--load-path` flag in Sass load path resolution * Add specs for component stylesheets * Add specs for StylesheetHelper * Rename clipboard component stylesheet to simple scss * Render stylesheet_once_tags everywhere application is rendered * Fix component preview for per-component stylesheet * LG-8439: resolve conflict * LG-8439: address comments * Updating how gpo pending is set in dummy profile (#8434) * Updating how gpo pending is set in dummy profile * changelog [skip changelog] --------- Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com>

aduth commented May 10, 2023

View reviewed changes

aduth mentioned this pull request May 11, 2023

build-sass: Add support for ".scss" extension #8385

Merged

aduth marked this pull request as ready for review May 11, 2023 20:46

aduth force-pushed the aduth-component-stylesheets-2 branch from cb360e9 to 4032301 Compare May 12, 2023 13:45

zachmargolis approved these changes May 16, 2023

View reviewed changes

aduth mentioned this pull request May 17, 2023

build-sass: Create output directory if missing #8384

Merged

aduth added 8 commits May 18, 2023 08:53

Add support for per-component stylesheets

955c26c

changelog: Internal, Optimization, Reduce size of stylesheet assets for critical path

Forward required to use uswds-core conventionally

2d9cd0f

Override _uswds-core via internal load path

4e8739f

Remove unnecessary load path

f785649

Prioritize --load-path flag in Sass load path resolution

83144c6

Add specs for component stylesheets

3128eae

Add specs for StylesheetHelper

08b9f89

Rename clipboard component stylesheet to simple scss

dfe4e0e

aduth force-pushed the aduth-component-stylesheets-2 branch from 4032301 to dfe4e0e Compare May 18, 2023 12:55

aduth added 2 commits May 18, 2023 09:14

Render stylesheet_once_tags everywhere application is rendered

42ec06d

Fix component preview for per-component stylesheet

c4d62e8

aduth merged commit b9c2334 into main May 18, 2023

aduth deleted the aduth-component-stylesheets-2 branch May 18, 2023 19:58

aduth mentioned this pull request May 24, 2023

Fix spinner button animation for long action text #8472

Merged

aduth mentioned this pull request Jun 9, 2023

Move component stylesheets to load dynamically with render #8571

Merged

aduth added the performance label Oct 23, 2023

aduth mentioned this pull request Apr 5, 2024

Update component documentation to describe stylesheet auto-loading #10375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for per-component stylesheets#8375

Add support for per-component stylesheets#8375
aduth merged 10 commits intomainfrom
aduth-component-stylesheets-2

aduth commented May 10, 2023 •

edited

Loading

Uh oh!

aduth May 10, 2023

Uh oh!

aduth May 11, 2023

Uh oh!

aduth May 11, 2023

Uh oh!

aduth May 11, 2023

Uh oh!

zachmargolis commented May 10, 2023

Uh oh!

aduth commented May 11, 2023 •

edited

Loading

Uh oh!

aduth commented May 11, 2023

Uh oh!

zachmargolis commented May 11, 2023

Uh oh!

zachmargolis commented May 11, 2023

Uh oh!

zachmargolis commented May 11, 2023

Uh oh!

zachmargolis left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

aduth commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 Summary of changes

📜 Testing Plan

Uh oh!

aduth May 10, 2023

Choose a reason for hiding this comment

Uh oh!

aduth May 11, 2023

Choose a reason for hiding this comment

Uh oh!

aduth May 11, 2023

Choose a reason for hiding this comment

Uh oh!

aduth May 11, 2023

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented May 10, 2023

Uh oh!

aduth commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented May 11, 2023

Uh oh!

zachmargolis commented May 11, 2023

Uh oh!

zachmargolis commented May 11, 2023

Uh oh!

zachmargolis commented May 11, 2023

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aduth commented May 10, 2023 •

edited

Loading

aduth commented May 11, 2023 •

edited

Loading