Skip to content

Restructure output as package overrides#336

Merged
aduth merged 4 commits intomainfrom
aduth-uswds-3-packages
Apr 17, 2023
Merged

Restructure output as package overrides#336
aduth merged 4 commits intomainfrom
aduth-uswds-3-packages

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 14, 2023

This pull request proposes a significant overhaul to the source and output file structure for the project, with two main objectives:

  1. Alignment to USWDS v3 in downstream project usage
  2. Alignment to USWDS v3 in internal package file structure

For usage, a goal here is that @18f/identity-design-system can act largely as a drop-in replacement to @uswds/uswds, where a consuming project could follow the USWDS developer guidance, and only need to substitute the package names.

For file structure, this exposes the new optimization options for per-package imports, as well as creating a more rigid one-to-one mapping between our overrides and the default design system stylings (which was already a goal documented in CONTRIBUTING.md).

How it works:

Essentially, this creates an output packages directory which contains all of the default USWDS packages directories. For packages we intend to customize, a file is added to the root of the packages directory, containing a reference to the original package as well as any customizations. This way, if a consuming project imports a package by name (e.g. @forward 'usa-alert';), the request will be intercepted if customizations exist, and otherwise load the default USWDS package.

The process for this is quite simple, as it copies the USWDS folders, then adds in our customizations:

https://github.com/18F/identity-style-guide/blob/5a8d45db0ba4333f70480896eaf20af33b5ac6c0/Makefile#L61-L62

Usage:

I updated a relevant section in README.md, though I would like to do a more substantial rewrite of the README prior to a final release.

You can also reference the fully-functional IdP branch at 18F/identity-idp#8093, which has now been updated to use this revised approach. Notably, where previously we had to juggle both @18f/identity-design-system and @uswds/uswds package references, the usage is much more streamlined now.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Base automatically changed from aduth-uswds-3 to main April 17, 2023 18:02
@aduth aduth mentioned this pull request Apr 17, 2023
@aduth aduth force-pushed the aduth-uswds-3-packages branch from 8b2f69c to d814cbe Compare April 17, 2023 18:05
Copy link
Contributor Author

@aduth aduth Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future Andrew: Ideally this file would be more minimalistic, forwarding the default package and adding our custom components, e.g.:

@forward './uswds/index';
@forward 'usa-code';
@forward 'usa-spinner';
@forward 'usa-verification-badge';

This would also make it more obvious which of these are custom to our design system, vs. extended from USWDS.

The reason we can't do that currently is because uswds/_index.scss references deep paths of the components that it forwards (source), which bypasses our technique for intercepting the root package handle.

Couple options to consider:

  • Upstream patch to USWDS to change uswds/_index.scss to reference the top-level handle (I don't see why it couldn't?)
  • Some other automation to copy the source uswds/_index.scss file to remove /src/styles file path references.
  • Try to intercept the deep path handles, such as the same technique for overriding the top-level handle, but applied for [package]/src/_styles.scss.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up at uswds/uswds#5241

Comment on lines +251 to +253
%block-input-styles {
margin-top: units(0.5);
}
Copy link
Contributor Author

@aduth aduth Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rearrangement of this may have inadvertently fixed an existing bug with input margins.

Originally flagged in the visual regression tests on the Form Fields page:

image

On the live site, previously the top margin was previously 0.5rem (incorrect):

Screen Shot 2023-04-17 at 2 33 59 PM

On this branch, the top margin is now halved to 0.25rem (correct):

Screen Shot 2023-04-17 at 2 34 20 PM

For reference, the Figma Design System document shows 4px (0.25rem) top margin:

image

cc @nickttng

@aduth
Copy link
Contributor Author

aduth commented Apr 17, 2023

I confirmed locally that the only visual regression is the one flagged by the build, explained at #336 (comment).

@aduth aduth merged commit ad56e4e into main Apr 17, 2023
@aduth aduth deleted the aduth-uswds-3-packages branch April 17, 2023 19:08
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.

2 participants