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

Responsive image aspect ratio variants #5308

Merged
merged 14 commits into from
Aug 20, 2024
Merged

Responsive image aspect ratio variants #5308

merged 14 commits into from
Aug 20, 2024

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Aug 16, 2024

Done

Adds new -on-(large|medium|small) variants of each aspect ratio image container class for more precise control over aspect ratios, depending on the screen size.

Fixes #5274, WD-13920

QA

  • Open the responsive-all example and verify that the aspect ratio labels above each example there are correct by resizing the window.
    • Make sure to check at the breakpoints. As long as there are not two classes for the same breakpoint, no two responsive aspect ratio classes should be active.
      • Large: width >=1036px
      • Medium: 620px >= width < 1036px
      • Small: width < 620px
  • Verify the responsive examples were added to the combined image example
  • Review image container with aspect ratio docs
  • Review 4.16.0 release notes

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

@webteam-app
Copy link

@jmuzina
Copy link
Member Author

jmuzina commented Aug 16, 2024

I'm not sure what the best way to document the list of aspect ratio classes is here, since there are so many of them (one for each combination of breakpoint and aspect ratio, and another one that applies to all aspect ratios)

I've added a table similar to the Grid page:

image

As well as an entry in the class-refs table:
image

@jmuzina jmuzina marked this pull request as ready for review August 16, 2024 21:34
@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Aug 19, 2024

I'm not sure what the best way to document the list of aspect ratio classes is here, since there are so many of them (one for each combination of breakpoint and aspect ratio, and another one that applies to all aspect ratios)

A table is good enough, but I would widen the first column to prevent wrapping.

Also, might be good to add some explanation as to why we do this - for example, mention that tall aspect ratios can cause a lot of scrolling on mobile, so a change to a less wasteful aspect ratio can be useful. on the other hand, a cinematic format is great when there's sufficient width (at least 9 columns), but for smaller screens, switching to 16x9 or 3x2 might allow more detail to be visible.

+1 for easier merging once the above is added

@jmuzina jmuzina marked this pull request as ready for review August 19, 2024 19:20
@jmuzina
Copy link
Member Author

jmuzina commented Aug 19, 2024

A table is good enough, but I would widen the first column to prevent wrapping.

I've updated the table so that the first column does not wrap on large & medium screens.

Screenshot 2024-08-19 at 3 21 50 PM

Also, might be good to add some explanation as to why we do this - for example, mention that tall aspect ratios can cause a lot of scrolling on mobile, so a change to a less wasteful aspect ratio can be useful. on the other hand, a cinematic format is great when there's sufficient width (at least 9 columns), but for smaller screens, switching to 16x9 or 3x2 might allow more detail to be visible.

I've added an explanation for reasons to use these responsive classes.
Screenshot 2024-08-19 at 3 22 31 PM

@jmuzina jmuzina marked this pull request as draft August 20, 2024 18:29
@jmuzina jmuzina marked this pull request as ready for review August 20, 2024 18:32
@jmuzina
Copy link
Member Author

jmuzina commented Aug 20, 2024

Updated to use inequalities rather than @media ((min|max)-width), as with #5315

@jmuzina

This comment was marked as resolved.

@jmuzina jmuzina marked this pull request as draft August 20, 2024 18:49
@jmuzina jmuzina marked this pull request as ready for review August 20, 2024 18:55
pastelcyborg
pastelcyborg previously approved these changes Aug 20, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Aug 20, 2024

@pastelcyborg I needed to merge main to resolve release notes conflicts - can you re-approve and also review Percy build please?

Note that image combined example is now screenshotted responsively to test the -on-medium classes.

@jmuzina jmuzina merged commit 75e90fa into main Aug 20, 2024
11 checks passed
@jmuzina jmuzina deleted the aspect-ratio-responsive branch August 20, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add breakpoint-specific image container aspect ratio classes
4 participants