cloudfour.com-patterns icon indicating copy to clipboard operation
cloudfour.com-patterns copied to clipboard

Add aspect ratio CSS utility classes, remove Embed object

Open gerardo-rodriguez opened this issue 1 year ago • 10 comments

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 Screen Shot 2022-09-22 at 1 13 01 PM

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

gerardo-rodriguez avatar Sep 16 '22 21:09 gerardo-rodriguez

🦋 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

changeset-bot[bot] avatar Sep 16 '22 21:09 changeset-bot[bot]

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Sep 16 '22 21:09 netlify[bot]

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

tylersticka avatar Sep 16 '22 22:09 tylersticka

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!

gerardo-rodriguez avatar Sep 19 '22 11:09 gerardo-rodriguez

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 avatar Sep 19 '22 18:09 gerardo-rodriguez

@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.

tylersticka avatar Sep 19 '22 18:09 tylersticka

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 avatar Sep 19 '22 18:09 tylersticka

@tylersticka @cloudfour/dev This PR has been reopened and is ready for review. Thank you! 😄

gerardo-rodriguez avatar Sep 22 '22 20:09 gerardo-rodriguez

@tylersticka @cloudfour/dev Thanks for your patience! I got myself all tied up in a knot, but I think I got it now!

gerardo-rodriguez avatar Sep 24 '22 00:09 gerardo-rodriguez

Moving back to "draft" until feedback is addressed.

gerardo-rodriguez avatar Sep 27 '22 20:09 gerardo-rodriguez

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.

tylersticka avatar Mar 15 '23 23:03 tylersticka