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

USWDS - Utilities: use CSS aspect-ratio #4811

Merged
merged 38 commits into from
Jul 18, 2022
Merged

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Jun 14, 2022

Draft Release Notes

Integrated the CSS aspect-ratio property into our add-aspect utility and mixins. This change aligns us more closely with expected CSS behavior, while also extending the capabilities of add-aspect. Users can now apply add-aspect directly to media and media wrappers — without the need to use .usa-embed-container. This update is backwards compatible and was written to preserve previous behavior for browsers that do not support the CSS aspect-ratio property.

Description

Closes: #4244

Preview link: Add aspect test scenarios

Problem

Our add-aspect utility does not behave in the same way that the native CSS aspect-ratio behaves. CSS aspect-ratio uses more straightforward CSS and also has increased capabilities. Because this CSS property is becoming increasingly accepted (Can I Use info), this discrepancy will likely cause increasing confusion to users.

Solution

Using the CSS aspect-ratio property in our add-aspect utility will create a more predictable experience for the user, as well as add some capability that our current setup does not allow.

Added capabilities:

  • Can add aspect ratio to both <img> wrapper elements and also directly to <img> elements
    • Key result: Aspect ratio can now be applied to media on our components, including hero, collection, media-block, etc
  • Adds ability to create height/width overrides (More info from CSS-tricks)
  • Deprecates need for adding .usa-embed-container in addition to add-aspect-xxx

Before/After comparison

For reference, you can see the preliminary side-by-side demonstration of the additional use cases in this demo of the old vs new functionality (built in a separate PR).

Additionally, for comparison here is a screenshot of the same test page markup from this PR with our current add-aspect build in develop (as seen on Chrome):

localhost_6006_iframe html_id=components-add-aspect--test viewMode=story (1)

⚠️ There is some inconsistency in implementation of aspect-ratio across browsers. However, the inconsistencies/failures are on use cases that our add-aspect ratio does not currently cover, so users will not be losing any capability.
- Tested in: Safari, Firefox, Chrome, iOS 15 safari, Android 11 chrome,
Found browser differences:

  • Android struggles to preserve ratio when height attribute is added:

    image

Tasks

  • Evaluate the use of CSS aspect-ratio property
  • Integrate CSS aspect-ratio into utilities
  • Integrate CSS aspect-ratio into mixins
  • Evaluate and improve (where possible) storybook presentation for embed-container and add-aspect-ratio
  • Write/improve documentation on how to use .add-aspect-AxB (Will create once we solidify code)

Testing

To test utility classes:

  1. Open the Add aspect test story
  2. Evaluate aspect ratios and visual appearance on each use case
  3. Assess if there are other possible use cases not included in the test

To test mixins:

  1. Uncomment these lines in add-aspect.scss:

    .test-add-aspect {
       @include at-media-max(desktop){
          @include add-aspect("1x1");
       };
       @include at-media(desktop){
          @include add-aspect("16x9");
       };
    }
    
  2. Then, either:

    <div class="test-add-aspect bg-secondary height-mobile"></div>
    <img class="test-add-aspect-image" alt="Alt text" src="https://designsystem.digital.gov/img/introducing-uswds-2-0/built-to-grow--alt.jpg">
    
  3. Notice that the element changes from 16x9 to 1x1 at the desktop breakpoint.

Additional information

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

@amyleadem amyleadem changed the title USWDS - add aspect: use CSS aspect-ratio USWDS - Utilities: use CSS aspect-ratio Jun 14, 2022
@amyleadem amyleadem marked this pull request as ready for review June 17, 2022 22:05
@amyleadem
Copy link
Contributor Author

@mejiaj I haven't updated the documentation yet (waiting for code to finalize), but I was hoping you could look over the code and let me know if you have any questions or concerns. It ended up being a little more layered than expected, so let me know if it would be helpful to chat through it.

@amyleadem amyleadem requested a review from mejiaj June 17, 2022 22:05
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Overall, I like the approach of using modern CSS techniques. Do you imagine this would be a breaking change for users?

I imagined the approach would be a mix of old code and new (using @supports for the newer CSS feature).

@amyleadem
Copy link
Contributor Author

@mejiaj
I have added @supports statements to inject the new aspect-ratio property where appropriate. It does make for a bit more redundant code, but it does feel safer.

I also have consolidated the aspect-ratio code inside the add-aspect mixin wherever possible so that we can easily control precedence for and apply add-aspect styles.

This has meant removing styles from the $add-aspect utility builder. I was wondering - is there any reason to keep this code in this updated/near-empty state? As a test, I have commented this code out and it has no impact on the CSS.

Additionally, I have done my best to answer the questions in your comments. If you have any questions or comments, please let me know!

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed answers! I like how you used @supports to enhance aspect ratios where applicable.

Added a few comments for deprecating features. Also added a question for hero structure with add-aspect.


This has meant removing styles from the $add-aspect utility builder. I was wondering - is there any reason to keep this code in this updated/near-empty state? As a test, I have commented this code out and it has no impact on the CSS.

I think it's only needed for generating the utility classes. It makes sense to keep it for now.

@amyleadem
Copy link
Contributor Author

@mejiaj I have made the following updates:

  1. Deprecated @mixin embed-container by moving the mixin code to _deprecated.scss and removing the related file and reference 4d4ed11
  2. Moved the vertical padding from .usa-hero to .usa-hero > .grid-container so that callout appearance would be consistent if add-aspect was applied. d5f2167

Thanks for the helpful comments! Please let me know if you see anything else.

@amyleadem amyleadem requested a review from mejiaj July 14, 2022 21:51
@amyleadem
Copy link
Contributor Author

@mejiaj Afterthought: Is it safe to deprecate the entire usa-embed-container package? It really just houses some sass and a storybook demo. I imagine the sass can be moved to _deprecated.scss and the demo could be moved to the add-aspect test page. I have the change ready to go but hesitated before pushing it up. Let me know what you think!

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Code is approved, nice work!

Since this is a big change to how aspect works, could you try to write up the draft release note for it? Anywhere in the PR description is fine, ty!

@amyleadem
Copy link
Contributor Author

Since this is a big change to how aspect works, could you try to write up the draft release note for it? Anywhere in the PR description is fine, ty!

Thanks @mejiaj. Wrote up a first pass at a draft release note. Please feel free to edit if you see the need.

Copy link
Member

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

@thisisdano thisisdano merged commit e8e37c7 into develop Jul 18, 2022
@thisisdano thisisdano deleted the al-aspect-ratio-use branch July 18, 2022 04:14
@thisisdano thisisdano mentioned this pull request Aug 5, 2022
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.

add-aspect not properly setting aspect ratio
3 participants