USWDS - Prose: Placeholder fix. Resolves #5094

[mahoneycm]

Alternative solution from #4962

Summary

Reverts usa-prose placeholders to use typeset mixins to resolve compilation errors.

Breaking change

⚠️ This is potentially a breaking change. This PR removes usa-prose- mixins and placeholders. Users should instead use typeset- mixins in their place.

Site changelog PR

uswds/uswds-site#2095

Related issue

Closes #4962

Preview link

Prose →

Problem statement

This work resolves two separate issues:

1. Build errors while extending %usa-prose- placeholders.
2. Compilation errors with mixins nested under the & > selector

1: Build errors with placeholder:

Sass can throw errors when we rely on @extend and placeholders in certain situations.
From #4962:

I’m trying to configure USWDS inside an @layer rule using the load-css function from sass:meta. When I try to @use "uswds-core" in my own Sass modules so that I can access the tokens for my styles, I get a target selector not found error for %usa-prose-p.

2: Compilation errors with mixins nested under the & > selector

This would result in the unexpected assignment of the & selector and in turn, incorrectly attach styles to classes

Solution

1. Revert usa-prose placeholder to a mixin
2. Manually paragraph and heading styles from the typeset mixins and directly into the usa-prose style definitions
3. Removed redundant and unused usa-prose mixins in favor of typeset- mixins

From @amyleadem in #5043:
Sass seems to get confused when * + & is brought in from another file via a mixin (more details in #4576). Additionally, Sass can throw errors in limited circumstances when we rely on @extend and placeholders.

The key conclusion is that * + & does not currently seem to be a viable addition to mixins or placeholders. By moving all * + & out of mixins and placeholders and directly into the style definitions, we can remove the risk of Sass confusion.

Note: The downside of this solution is that it requires some code repetition, but that might be slightly mitigated by creating variables/settings for the heading and paragraph top margins.

Major changes

This PR removes usa-prose- mixins and placeholders. Users should instead use typeset- mixins in their place.

List of mixins removed:

• usa-prose-link
• usa-prose-display
• usa-prose-p
• usa-prose-heading
• usa-prose-h1 through h6

New settings variables

• $theme-heading-margin-top
• $theme-paragraph-margin-top

Testing and review

1. Confirm that Sass compiles without error
2. Visit Prose Storybook
3. Inspect usa-prose styles and make sure * + & selectors are correctly targetting headings and p tags
4. Use npm link to create a symlink to the package
5. Clone this test repo
6. Use npm link @uswds/uswds to replace USWDS package with the symlink
7. Run npm build and confirm no build errors

• Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
• Run npm run prettier:scss to format any Sass updates.
• Run npm test and confirm that all tests pass.

[mahoneycm]

@mejiaj @amyleadem This is the alternative solution mentioned in James' comment

This maintains current develop styles but it does move them in the output CSS.

Here is an easel that shows the relevant difference between files and verifies all necessary prose styles are present.: usa prose refactor

I tried adding colored arrows to help connect the changes

[amyleadem]

Hi @mahoneycm, I have started my review but need to step away for a bit. In the meantime, I found an error when $theme-global-content-styles is set to true because it is looking for the now missing usa-prose-link. I've also added some comments below with notes and a couple questions about code redundancy. I will cycle back later, but let me know if you have questions in the meantime.

So far the new solution appears to be on track, but there just a couple things to look at.

[amyleadem]

Curious - this appears to be redundant with a good chunk of the typeset-p mixin. Is there a way to reduce redundancy without having a negative impact?

[mahoneycm]

I haven't been able to successfully use the typeset mixins without it incorrectly setting *+.usa-prose p as the top-margin selector, unfortunately.

[mahoneycm]

Scratch that, I'm going to use @mejiaj's typset-base recommendation to alleviate some of the redundancy!

[amyleadem]

Same thing here - feels redundant with the rules with typeset-heading. Can we reduce code redundancy here?

[mejiaj]

This solution outputs what I would expect. If we go this route we should consider adding guidance to users (if any) that were using the removed extends.

---

Alternatively, we should consider simplifying in the future. By doing something like:

.usa-prose {
  & > {
    @include usa-list-styles;
    @include usa-table-styles;

    // Adds consistent margin to *all* child elements
    * + * {
      margin-top: $theme-paragraph-margin-top !important;
    }

    …H1-H6 styles

    // Only override headers
    * + {
      h1,
      h2,
      h3,
      h4,
      h5,
      h6 {
        margin-top: $theme-heading-margin-top !important;
      }
    }
  }
}

[mejiaj]

To avoid duplication, you could create a typeset-heading-base and typeset-p-base that include these styles in typeset.scss.

//
// typeset.scss
//
@mixin typeset-heading-base {
  @include u-margin-y(0);
  clear: both;
}

@mixin typeset-heading {
  @include typeset-heading-base;

  * + & {
    margin-top: $theme-heading-margin-top;
  }

  + * {
    margin-top: $theme-paragraph-margin-top;
  }
}

// typeset element mixins
@mixin typset-p-base {
  line-height: line-height($theme-body-font-family, $theme-body-line-height);
  margin-bottom: 0;
  margin-top: 0;
  max-width: measure($theme-text-measure);
}

@mixin typeset-p {
  @include typset-p-base;

  * + & {
    margin-top: $theme-paragraph-margin-top;
  }

  + * {
    margin-top: $theme-paragraph-margin-top;
  }
}

[mahoneycm]

I like this idea. I think it'll clean up the usa-prose class nicely!

[mahoneycm]

I was able to use the typset-p/heading-base mixin and I personally like the clean-up it offers!

Using * + * did not work as expected but I was able to use *:not(:first-child) to achieve the same result

[mahoneycm]

@mejiaj @amyleadem ok! So I've completed some refactoring by a combination of creating typeset-heading-base and typeset-p-base to avoid some duplication and to correctly reset top and bottom margins. I also moved certain selectors around in order to have the styles apply correctly and used the owl selector * + * to correctly give all siblings of usa-prose children the correct top margin.

I've taken a stab at some basic SASS unit testing to make sure that our nested selectors are working as expected! I'd love your feedback whenever you have a chance to see if there are other enhancements I may be missing, or if we can test further with the unit tests!

[mejiaj]

Looks good, minor comments!

[amyleadem]

@mahoneycm This is looking good! It is feeling cleaner with each iteration. Thanks a ton for hanging in there with this one. Only thing I have to flag is the failed Sass test that is causing the build error.

• Confirm that vertical margins look correct in the  usa-prose story
• Confirm that vertical margins look correct when $theme-global-paragraph-styles and $theme-global-content-styles are set to true
• Confirm that * + & references have been accounted for in the compiled CSS
• Confirm that the error is resolved in the test repo
• Confirm that updating values in $theme-heading-margin-top and $theme-paragraph-margin-top updates relevant values.
• Confirm npm start runs without error
• Confirm npm test runs without error
  • Receiving an error with the prose Sass test
• Confirm gulp buildSass runs without error
• Confirm no errors when $theme-global-content-styles is set to true
• Check visual display on Safari, Firefox, Edge, Chrome

[mejiaj]

@mahoneycm can you please look into the failing test?

[mahoneycm]

@mejiaj @amyleadem Looks like when I broke it when I tried formatting the test scss. I believe it was only because of the order of the output not matching the expected order. Moving the expected results to match the actual outputs did the trick!

[amyleadem]

Thanks for fixing the test!

[mahoneycm]

@thisisdano Ready for final review!

[thisisdano]

I'll draw up a changelog PR for this change.
Changelog PR: uswds/uswds-site#2096

[mahoneycm]

@thisisdano I created a changelog of this one before I left yesterday as well, let me know if you want me to go ahead and close this ticket: uswds/uswds-site#2095

I also wanted to note I did mark it as a breaking change because it removes the usa-prose- mixin.