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

Chore: Adds embed object mixin #754

Merged
merged 21 commits into from
Aug 6, 2020
Merged

Conversation

derekshirk
Copy link
Contributor

@derekshirk derekshirk commented May 28, 2020

Overview

This PR was originally intended to address the needs of styling the Gutenberg core/image block. After further effort I realize that the work represented here thus far, no longer meets the needs of that goal.

However, the embed object mixin may still be a meaningful addition. This pull request accomplices the following:

  • adds mixin for embed object
  • updates o-embed to make use of available mixin

I am opening this up for review as is. What do you think? Are these changes worth introducing even though they aren't aligned with the original intent of styling the Gutenberg core/image block?

Gutenberg core/image block styles will be addressed and added via #802

Testing

  1. View deploy preview
  2. Review embed object output
  3. Confirm no regressions have been introduced

See #710

@derekshirk derekshirk self-assigned this May 29, 2020
@derekshirk
Copy link
Contributor Author

👋 @tylersticka @spaceninja

I was wondering if either of you would be able to review the current state of this PR, which only introduces a change that converts the existing CSS styles for o-embed into a mixin.

My primary question is if I handled the @supports rule appropriately.

Prior to these changes the @supports rule was wrapping the (non-fallback) .o-embed styles.

@supports (--custom: property) {
/**
* Here's where the magic happens! 🦄
*
* 1. We default to a square aspect ratio. This is suitable for icons, profile
* images, etc. It's also the simplest default.
* 2. Required for rounded corners to work as expected. We could apply this
* by default, but then circular rounding would appear elliptical in older
* browsers.
* 3. Required to absolute-position child elements.
*/
.o-embed {
--aspect-ratio: #{$default-aspect-ratio}; /* 1 */
overflow: hidden; /* 2 */
position: relative; /* 3 */

For the new mixin, I inverted these:

@mixin object {
  @supports (--custom: property) {
      ...

@tylersticka
Copy link
Member

@derekshirk I tried it out locally and it seems fine to me.

The way I tested was that I changed the @supports query to some nonsense (I believe I used floob: whut), confirmed the fallback styles were applied, then reverted the query to what it was before and confirmed the enhanced styles were applied.

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2020

💥 No Changeset

Latest commit: b7f2bff

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@derekshirk
Copy link
Contributor Author

derekshirk commented Jun 4, 2020

I will update/add the Changeset once this is ready for review.

As I understand it, this PR does not require a changeset.

@derekshirk derekshirk changed the title Chore: styles Gutenberg image block Chore: Adds embed object mixin Jun 18, 2020
@derekshirk derekshirk requested a review from a team June 18, 2020 23:29
@derekshirk derekshirk marked this pull request as ready for review June 18, 2020 23:29
Copy link
Member

@spaceninja spaceninja left a comment

Choose a reason for hiding this comment

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

Unless I'm reading this wrong, this will result in no actual changes to the compiled CSS, so no objections here.

@derekshirk derekshirk merged commit bb5ae55 into v-next Aug 6, 2020
@derekshirk derekshirk deleted the chore/gutenberg-core-image branch August 6, 2020 18:38
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.

3 participants