cloudfour.com-patterns
cloudfour.com-patterns copied to clipboard
Add aspect ratio CSS utility classes, remove Embed object
Overview
This PR does the following:
- Introduces
u-aspect-*
CSS utility classes - Removes the Embed object
Screenshots
Left: Before Right: After
![Screen Shot 2022-09-22 at 1 12 45 PM](https://user-images.githubusercontent.com/459757/191842058-20db68c0-5eb3-4d84-8952-8a3e103b5ffb.png)
![Screen Shot 2022-09-22 at 1 13 01 PM](https://user-images.githubusercontent.com/459757/191842071-0cba4492-b377-4b35-855e-c516ba8db20d.png)
Testing
Before: https://cloudfour-patterns.netlify.app/?path=/docs/objects-embed--image After: https://deploy-preview-2053--cloudfour-patterns.netlify.app/?path=/docs/utilities-aspect-ratio--image
Check all browsers! Especially Safari. 😅
- [ ] Confirm all four aspect ratio utility classes work (
square
,full
,wide
,cinema
) - [ ] Ensure the
--aspect-ratio
CSS custom property works - [ ] Compare before/after, there should be no visual differences
- Closes #1999
🦋 Changeset detected
Latest commit: 22d650855e8d0ec3b0f58107e726d9017f240d60
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
Name | Type |
---|---|
@cloudfour/patterns | Major |
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
Deploy Preview for cloudfour-patterns ready!
Name | Link |
---|---|
Latest commit | 22d650855e8d0ec3b0f58107e726d9017f240d60 |
Latest deploy log | https://app.netlify.com/sites/cloudfour-patterns/deploys/6389170ed0092500085388a5 |
Deploy Preview | https://deploy-preview-2053--cloudfour-patterns.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
I haven't tested thoroughly, but this PR seems to do a good job of swapping the properties.
I think the bigger question I have is whether or not this property invalidates the need for this component entirely? Like, is there value in having the containing element at all?
Here's an example of where I replace the component with a utility on the embedded element directly: https://codepen.io/tylersticka/pen/zYjZdvB/491a9324047e4651d25bcf6a7bdefbb4
I think the bigger question I have is whether or not this property invalidates the need for this component entirely? Like, is there value in having the containing element at all?
Here's an example of where I replace the component with a utility on the embedded element directly: https://codepen.io/tylersticka/pen/zYjZdvB/491a9324047e4651d25bcf6a7bdefbb4
@tylersticka Interesting! I can look into making this a CSS utility class instead. Thanks!
I think the bigger question I have is whether or not this property invalidates the need for this component entirely? Like, is there value in having the containing element at all?
@tylersticka I'm creating a set of utility functions, but I just noticed a detail that makes me pause.
Is there value in having a containing element? Perhaps? What about hover interactions, for example, on the Card element?
If we directly give the image an aspect-ratio utility CSS class, it'll be the correct aspect ratio. But, on hover, the Card cover image doesn't have a wrapping element with overflow: hidden
, creating a bug.
I suppose I could just add an overflow: hidden
to the .c-card__cover
element. That seems like it should fix it.
Thinking "out loud." Do you have any thoughts on this, @tylersticka? Do you see it as an issue if I need to add overflow: hidden
to the .c-card__cover
element so that it can play nicely with an aspect-ratio utility CSS class?
@gerardo-rodriguez I think that's kind of what I was asking when I asked this:
Like, is there value in having the containing element at all?
For example, I think the Avatar component does not use the Embed object directly, so in that case it feels like the CSS could be self-contained. I wasn't sure if there are components like Card that rely on it and, if so, what that looks like.
We could also do a phased approach where we "deprecate" the Embed component and move over to the aspect ratio utilities slowly.
Or you could chat with others in @cloudfour/dev and determine that there's no harm in switching Embed over and I'm over-complicating things. Mainly I just want to make sure we've considered the usefulness of the component in a world without the padding hack.
Or you could chat with others in @cloudfour/dev and determine that there's no harm in switching Embed over and I'm over-complicating things.
Just to elaborate on this point: If you all decide that maybe in the future it might be useful to add other things to an embed, like an icon or overlay or some other visual element, then maybe it's fine to keep a component where the class is on the containing element.
I just want to be clear that I'm asking for confirmation that we've considered the component's continued usefulness and a recommendation for what to do next: My intent is not to provide specific direction right now.
Happy to chat with voices if that helps!
@tylersticka @cloudfour/dev This PR has been reopened and is ready for review. Thank you! 😄
@tylersticka @cloudfour/dev Thanks for your patience! I got myself all tied up in a knot, but I think I got it now!
Moving back to "draft" until feedback is addressed.
This PR is now several months old, and our embed styles are now shared between the embed component and multiple Gutenberg blocks. I'm going to close this PR in favor of re-approaching this with fresh eyes at a future date.