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

Ensure correct attributes are always used for embed preview #14748

Merged
merged 4 commits into from May 9, 2019

Conversation

@notnownikki
Copy link
Member

commented Apr 1, 2019

Description

When pasting a YouTube URL multiple times, the responsive styles are not applied on the first render, and so the second render is incorrect and either has scrollbars or whitespace.

This change make sure we are always rendering using the correct attributes, generated from the preview.

How has this been tested?

Paste a YouTube URL, e.g. https://www.youtube.com/watch?v=wSaoXwQzHnY multiple times into a new post. Each video should be responsive with no large amounts of whitespace or scrollbars.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@draganescu

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Howdy @notnownikki is there a particular browser this happens on, because I tested the way you described and there appears to be no bug on master.

On master, after pasting the YouTube PR multiple times I get this:

Screenshot 2019-04-03 at 12 50 42

On this PR, after pasting the YouTube PR multiple times I get this:

Screenshot 2019-04-03 at 12 51 10

What am I missing and how to test :)?

@notnownikki

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I'm using Chrome 73.0.3683.75, the bug might not appear when the window is narrow as you have it there.

Here's what I see on current master, stock WP, 2019 theme, no plugins, browser maximised at 1920x1080 resolution monitor:

whitespace

The wider the window, the worse that whitespace at the bottom gets.

@notnownikki

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Also tested in Firefox 66.0.2, and I see the same thing.

@notnownikki

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

On android chrome, there's a different symptom of the problem, you only see a small section of the video preview. Here's what it looks like pasting a YouTube URL on android chrome, current master:

Screenshot_20190403_111957_com android chrome

This PR fixes that.

@draganescu

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Yes @notnownikki I finally could see it happen :D my testing was wrong.

@draganescu

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Tested the code and it works as described, there is no more spacing being created for no reason. I have also rebased the branch because it required a useless npm install to test.

@draganescu draganescu force-pushed the fix/intermittent-responsive-resizing-fail branch from fcb7c46 to 4e8b1b7 May 2, 2019

@notnownikki

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

I have clarified the comment in render and renamed the methods that get the attributes from the preview and merge them with the block's current attributes. Hopefully that makes things clearer to new readers :)

@gziolo gziolo removed this from the 5.6 (Gutenberg) milestone May 6, 2019

@draganescu
Copy link
Contributor

left a comment

tested again, works great. this is good to go!

@draganescu draganescu merged commit f628a8a into master May 9, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@draganescu draganescu added this to the 5.7 (Gutenberg) milestone May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.