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-Site - Card: Fix card grid presentation #2299

Merged
merged 21 commits into from
Dec 1, 2023
Merged

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Oct 6, 2023

Summary

  • Switched out the grid classes in the card preview template. This improves the appearance of the card component preview at narrow widths.
  • Added grid styles specifically suited for the card component preview.
  • Added a changelog entry for card grid fix.

Related issue

Related to #2323

Preview link

Problem statement

The card component preview page shows cards that get too narrow and elements within the cards overlap at narrow browser widths.

image

Solution

Adjusting the grid breakpoints in the card template improves the display of the component at narrow widths.

This fix currently provides specific grid classes for only for the scenarios needed. Alternatively, we could turn on the "widescreen" and "tablet-lg" values in $theme-utility-breakpoints. However, this felt heavy-handed for this specific need (See more info in this comment).

Testing and review

  • Confirm the card component preview presents well at various breakpoints and has the correct grid breakpoints.
  • Confirm that the approach for updating the styles makes sense.
  • Confirm the changelog is accurate and free from spelling and grammatical errors.

@amyleadem amyleadem marked this pull request as ready for review October 6, 2023 19:07
@amyleadem amyleadem requested review from mahoneycm and mejiaj October 20, 2023 16:12
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Ran into an issue trying to get my local to build. Also just left a question about the approach and the alternative you mentioned in the description.

package.json Outdated Show resolved Hide resolved
css/_uswds-theme-custom-styles.scss Show resolved Hide resolved
css/_uswds-theme-custom-styles.scss Outdated Show resolved Hide resolved
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.

Added a possible alternative to adding more to custom SCSS.

css/_uswds-theme-custom-styles.scss Outdated Show resolved Hide resolved
@amyleadem
Copy link
Contributor Author

amyleadem commented Oct 25, 2023

@mejiaj @mahoneycm
I have updated the grid classes in this repo and removed the USWDS test branch. I think we can now safely remove this item from the 3.7.0 release.

I added a note about why I didn't turn on the breakpoint via settings in this comment. Let me know if you see a better way to do this.

@amyleadem amyleadem requested review from mahoneycm and mejiaj October 25, 2023 22:02
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Hey Amy!

I found a way to achieve the same styles using the string replacement without the need for any scss changes. This route resolves the build errors and isolates the changes.

Let me know what you think about this approach!

_includes/code/components/card.html Show resolved Hide resolved
_includes/code/components/card.html Outdated Show resolved Hide resolved
css/_uswds-theme-custom-styles.scss Outdated Show resolved Hide resolved
@amyleadem
Copy link
Contributor Author

The comment history here has gotten a bit jumbled, but I wanted to summarize the recent changes in this PR:

  • I kept the tablet-lg:grid-col-6 widescreen:grid-col-4 classes on card--standard. These grid breakpoints are not included in our breakpoint list, which means that we need copy over a couple of grid style rules from USWDS. However, I felt that these were the ideal breakpoints for the standard cards, and since it is a commonly-used component, I wanted to showcase them in their best light.
    • Alternatively, we could use tablet:grid-col-6 desktop-lg:grid-col-4, which are already accounted for in the CSS. No additional CSS rules would be required if we chose these classes instead, but the cards do end up getting a bit narrow at various widths.
    • Alternatively, we could also pursue custom site-specific class names here instead of copying over USWDS rules, but I went with the standard USWDS grid classes so that users would be able to use the code if they copied it from web inspector.
    • We could also turn these breakpoints in the $theme-utility-breakpoints setting, but this would add a couple thousand more references and 100kb+ to the compiled CSS.
  • These grid classes do not meet BEM standards, so I had to exclude them from stylelint checks by using stylelint-disable comments.
  • I changed the card--flag classes to flex-1 as @mahoneycm suggested. This allowed for the desired presentation without needing to add an additional rule definition for .widescreen\:grid-col-6.

@amyleadem amyleadem requested a review from mahoneycm November 20, 2023 19:58
@amyleadem
Copy link
Contributor Author

@mejiaj @mahoneycm ready for your re-review. Let me know if you have comments.

Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Lgtm!

As long as we're ok with maintaining the CSS overrides, I think that this is a visual improvement to showcase cards 👍

  • Cards break at logical breakpoints
  • Cards do not feel two thing at any point
  • No errors when running npm run build:css

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.

LGTM, glad we were able to agree on a fix. Good to merge once builds pass.

@amyleadem amyleadem merged commit aa5eacf into main Dec 1, 2023
4 checks passed
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.

USWDS-Site - Bug: Flag Layout Cards overlapping
3 participants