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

Updates responsive embeds with new class names and CSS variables #31717

Merged
merged 5 commits into from
Sep 23, 2020

Conversation

mdo
Copy link
Member

@mdo mdo commented Sep 21, 2020

  • Renames .embed-responsive-XbyY classes to .embed-responsive-XxY
  • Simplifies the Sass map to just be key and value, ratio and percentage
  • Builds .embed-responsive-* modifiers with CSS variables
  • Updates docs to show power of CSS variables
  • Add notes to the Migration guide

Fixes #31531.


Screen Shot 2020-09-21 at 2 15 25 PM

Screen Shot 2020-09-21 at 2 15 30 PM

- Renames .embed-responsive-XbyY classes to .embed-responsive-XxY
- Simplifies the Sass map to just be key and value, ratio and percentage
- Builds .embed-responsive-* modifiers with CSS variables
- Updates docs to show power of CSS variables
- Add notes to the Migration guide
scss/_variables.scss Outdated Show resolved Hide resolved
@MartijnCuppens
Copy link
Member

Thoughts about renaming embed-responsive to ratio?

@mdo
Copy link
Member Author

mdo commented Sep 22, 2020

Think that'd be fine @MartijnCuppens. We'd change the .responsive-embed-item too, and keep the nested selectors, right?

@mdo
Copy link
Member Author

mdo commented Sep 22, 2020

Done, and done, and done! Latest commits rename it to .ratio and move inline styles to the docs CSS. Also added an example of using breakpoints to change the CSS variable for responsive ratios.

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM!

@MartijnCuppens
Copy link
Member

Very nice

One last thought:

We could simplify this to just > *:

.ratio-item,
iframe,
embed,
object,
video {

It feels a bit inconsistent that we would require .ratio-item on a <div> or an <img />, but not on an <iframe> or a <video>

- Remove group selector for ratio items
- Drop the .ratio-item entirely
- Update docs to explain updated approach
- Update Migration guide to reflect the latest
@mdo
Copy link
Member Author

mdo commented Sep 23, 2020

Yup, good idea—just pushed some changes. Done!

@mdo mdo closed this Sep 23, 2020
@mdo mdo reopened this Sep 23, 2020
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

:shipit:

@mdo mdo merged commit 9f60659 into main Sep 23, 2020
@mdo mdo deleted the redo-responsive-embeds branch September 23, 2020 20:30
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 responsive embed class with ratio by variable
3 participants