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

Override USWDS Sass packages at source #356

Merged
merged 4 commits into from
Jun 9, 2023
Merged

Override USWDS Sass packages at source #356

merged 4 commits into from
Jun 9, 2023

Conversation

aduth
Copy link
Member

@aduth aduth commented May 9, 2023

This builds upon #336 to further restructure the folder structure to align to how files are managed in the upstream project.

Why?

Unfortunately the diff didn't turn out as clean as I'd hoped, despite the fact that 99% of changes are file renames.

Essentially, this converts:

Before:

src/
 - scss/
  - packages/
   - _usa-accordion.scss
// _usa-accordion.scss

@forward './usa-accordion/index';

.usa-accordion {
  // ...customizations...
}

After:

src/
 - scss/
  - packages/
   - _usa-accordion/
    - src/
     -  _styles.scss
     - _overrides.scss
   - _usa-accordion.scss
// _usa-accordion.scss

@forward 'usa-accordion/index';
@forward 'usa-accordion/src/overrides';
// usa-accordion/src/_styles.scss

@forward 'usa-accordion/src/styles/index';
@forward './overrides';
// usa-accordion/src/_overrides.scss

.usa-accordion {
  // ...customizations...
}

The same general principle of intercepting USWDS files applies, but now applies for both the top-level import (@forward 'usa-accordion') as well as the package source (@forward 'usa-accordion/src/styles').

The only meaningful code change here was to eliminate $border-width variable in some of the SCSS files. I was running into an error message that seemed to be caused by multiple forwarded modules each defining the variable. Unclear why this wasn't an issue previously 😬

Error: Two forwarded modules both define a variable named $border-width.
  ╷
1 │ @forward 'usa-checkbox/index';
  │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ original @forward
2 │ @forward 'usa-checkbox/src/overrides';
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ new @forward
  ╵
  _usa-checkbox.scss 2:1               @forward
  uswds-form-controls/_index.scss 4:1  @forward
  _uswds-form-controls.scss 2:1        @forward
  uswds/_index.scss 51:1               @forward
  _uswds.scss 2:1                      @forward
  src/scss/styles.scss 2:1             root stylesheet

aduth added 4 commits May 9, 2023 15:17
These are called from top-level in uswds-typography and uswds-form-elements
Cover all bases for how someone might import
@aduth
Copy link
Member Author

aduth commented May 10, 2023

I'm open to trying to break this down into a few separate pull requests to try to coerce git into showing a simpler diff, if it'd be helpful for review.

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

it might have been nicer as one PR per component or maybe even separate commits (one commit per compoonent) but the documentatino makes it clear.

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