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

Use best fitting embed aspect ratio if exact match doesn't exist #10213

Merged
merged 3 commits into from Sep 29, 2018

Conversation

Projects
None yet
5 participants
@notnownikki
Member

notnownikki commented Sep 27, 2018

Description

This picks the best fit of the common aspect ratios if an exact fit does not appear in our list, for responsive content.

Fixes: #8383 (comment)

How has this been tested?

Embed https://www.youtube.com/watch?v=XT13ijUfSts and check it has the 16-9 style.

Embed https://www.youtube.com/watch?v=uAE6Il6OTcs and check it has the 4-3 style.

Embed https://vimeo.com/139290912 and check it has the 21-9 style.

Embed https://vimeo.com/139290912 again in the same post, and check the new embed has the 21-9 style.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@notnownikki notnownikki requested review from tofumatt and jasmussen Sep 27, 2018

@notnownikki notnownikki added this to the 4.0 milestone Sep 27, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

This is great! It works well. Twenty Seventeen front:

screen shot 2018-09-28 at 09 44 30

Backend:

screen shot 2018-09-28 at 09 44 37

However that above is after a reload. There seems to be an issue with the Vimeo video where the 2nd and 3rd time you embed it, it's not properly sized:

repeat embed

I did a little inspecting, and as it turns out, the 2nd and 3rd vimeo embed is correctly identified same as the first one. However the right classes are not applied. Compare, this is the body tag of the sandbox iframe for the first vimeo embed:

screen shot 2018-09-28 at 09 53 20

Now compare with the body tag of the same sandbox iframe for the 2nd and 3rd video embeds:

screen shot 2018-09-28 at 09 53 03

Note how the first one has all these CSS classes:

video wp-block-embed-vimeo wp-embed-aspect-21-9 wp-has-aspect-ratio

the 2nd and 3rd one only has this one:

wp-block-embed-vimeo

Note that if you save the post, then reload, evertything is fine, as you can see in the first screenshot. The missing classes happen only on the first edit. The important CSS class we need to have is wp-has-aspect-ratio.

Any idea why the classes go missing the first time but appear after a reload?

Contributor

jasmussen commented Sep 28, 2018

This is great! It works well. Twenty Seventeen front:

screen shot 2018-09-28 at 09 44 30

Backend:

screen shot 2018-09-28 at 09 44 37

However that above is after a reload. There seems to be an issue with the Vimeo video where the 2nd and 3rd time you embed it, it's not properly sized:

repeat embed

I did a little inspecting, and as it turns out, the 2nd and 3rd vimeo embed is correctly identified same as the first one. However the right classes are not applied. Compare, this is the body tag of the sandbox iframe for the first vimeo embed:

screen shot 2018-09-28 at 09 53 20

Now compare with the body tag of the same sandbox iframe for the 2nd and 3rd video embeds:

screen shot 2018-09-28 at 09 53 03

Note how the first one has all these CSS classes:

video wp-block-embed-vimeo wp-embed-aspect-21-9 wp-has-aspect-ratio

the 2nd and 3rd one only has this one:

wp-block-embed-vimeo

Note that if you save the post, then reload, evertything is fine, as you can see in the first screenshot. The missing classes happen only on the first edit. The important CSS class we need to have is wp-has-aspect-ratio.

Any idea why the classes go missing the first time but appear after a reload?

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 28, 2018

Member

Any idea why the classes go missing the first time but appear after a reload?

That's really strange because this contains a fix specifically for that. I'll take a look again.

Member

notnownikki commented Sep 28, 2018

Any idea why the classes go missing the first time but appear after a reload?

That's really strange because this contains a fix specifically for that. I'll take a look again.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

Uhm, let me also clear cache and try again.

Contributor

jasmussen commented Sep 28, 2018

Uhm, let me also clear cache and try again.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 28, 2018

Member

Ugh, ok, so this does set the classes correctly, but it doesn't make it through to the edit's render. The front end published post is fine, but the edit has problems.

Member

notnownikki commented Sep 28, 2018

Ugh, ok, so this does set the classes correctly, but it doesn't make it through to the edit's render. The front end published post is fine, but the edit has problems.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

Okay I'm not insane.

It feels like we're 99% there, though ❤️

Contributor

jasmussen commented Sep 28, 2018

Okay I'm not insane.

It feels like we're 99% there, though ❤️

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 28, 2018

Member

Yeah, it's just that there are a number of issues with React components and the type of data updating we do here. I'm considering a refactor to make the edit component very simple, and move most of the logic into the part where we select the data from the store. But, looking at this now.

Member

notnownikki commented Sep 28, 2018

Yeah, it's just that there are a number of issues with React components and the type of data updating we do here. I'm considering a refactor to make the edit component very simple, and move most of the logic into the part where we select the data from the store. But, looking at this now.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 28, 2018

Member

Fix for the issue is in another PR, which currently has a conflict with master and so can't be reviewed and merged. I'll fix that PR, but there's nothing we can do in this one to fix the issue, it's to do with passing around attributes when the embed block changes type (which happens when you paste a URL) and there's a refactor in #10035 to fix that.

Member

notnownikki commented Sep 28, 2018

Fix for the issue is in another PR, which currently has a conflict with master and so can't be reviewed and merged. I'll fix that PR, but there's nothing we can do in this one to fix the issue, it's to do with passing around attributes when the embed block changes type (which happens when you paste a URL) and there's a refactor in #10035 to fix that.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

In that case I think this is definitely an improvement over we have, especially given how rare embedding the same thing twice is. Expanding for a quick code review.

Contributor

jasmussen commented Sep 28, 2018

In that case I think this is definitely an improvement over we have, especially given how rare embedding the same thing twice is. Expanding for a quick code review.

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 28, 2018

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 28, 2018

Member

The other PR will be fixed today and opened up for core review.

Member

notnownikki commented Sep 28, 2018

The other PR will be fixed today and opened up for core review.

@tofumatt

Code seems fine, just comments about the loop used and function names, but just ping me if my comments are unclear. If they make sense feel free to modify+merge 😄

more-red-than-green

There's talk of issues here but they're resolved in other PRs so if the UX here is better by @jasmussen I'm cool with the code.

Show resolved Hide resolved packages/block-library/src/embed/index.js Outdated
Show outdated Hide outdated packages/block-library/src/embed/index.js Outdated
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

It's a good step forward and since the final step is in progress separately, 👍👍

Contributor

jasmussen commented Sep 28, 2018

It's a good step forward and since the final step is in progress separately, 👍👍

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 28, 2018

Member

I'll defer to others more knowledgeable than I, but I don't necessarily agree with the premise of this pull request in assigning an aspect ratio to an element which is not in-fact of that ratio.

Member

aduth commented Sep 28, 2018

I'll defer to others more knowledgeable than I, but I don't necessarily agree with the premise of this pull request in assigning an aspect ratio to an element which is not in-fact of that ratio.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 28, 2018

Contributor

I'll defer to others more knowledgeable than I, but I don't necessarily agree with the premise of this pull request in assigning an aspect ratio to an element which is not in-fact of that ratio.

If we know for sure it's a video, then it needs SOME aspect ratio, all videos have them. If the video element behaved like an img, the height would be intrinsically applied.

We use this detected aspect ratio to apply a padding that's relatively easily calculated from the dimensions. So as an alternative to css classes we could apply this padding inline. But in the past, inline styles have caused headaches with themes, which is why we went this route.

Contributor

jasmussen commented Sep 28, 2018

I'll defer to others more knowledgeable than I, but I don't necessarily agree with the premise of this pull request in assigning an aspect ratio to an element which is not in-fact of that ratio.

If we know for sure it's a video, then it needs SOME aspect ratio, all videos have them. If the video element behaved like an img, the height would be intrinsically applied.

We use this detected aspect ratio to apply a padding that's relatively easily calculated from the dimensions. So as an alternative to css classes we could apply this padding inline. But in the past, inline styles have caused headaches with themes, which is why we went this route.

@tofumatt

Additional changes are good to me, thanks!

@notnownikki notnownikki merged commit 58fd2d1 into master Sep 29, 2018

2 checks passed

codecov/project 48.57% (-0.24%) compared to 8300578
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/embed-better-aspect-support branch Sep 30, 2018

@mtias mtias added the Embeds label Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment