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

Output width and height attributes for Image blocks #9421

Closed
wants to merge 10 commits into from

Conversation

kienstra
Copy link
Contributor

Description

Could you use this PR for #6652, which ensures that most Image block <img> elements will have a width and height?

Before, Image blocks usually only had width or height attributes for the <img> if the user edited the <input> elements for those, or clicked a percentage pillbox:

before-width-height

This PR sets the width and height on selecting an image from the Media Library, and on selecting from the 'Image Size' <select> element. Clicking a percentage pillbox, like 25%, still updates the width and height:

changing-these-width-height

The is-resized class
Before, there was an is-resized class that applied only when there were width or height attribute values. These values only existed on editing the Width and Height TextControls directly, or from clicking the percentage pillbox (shown in the first screenshot above).

But now, there are almost always width and height values, as they're added via the Media Library, and updated as shown in the second screenshot above.

So this adds the attribute isResized to apply the class is-resized in the same cases where it was applied before.

How has this been tested?

  1. Created a new Image block, and verified that it had width and height attributes, but no is-resized class

width-height-present

  1. Changed that image's 'Image Size' to Thumbnail as shown in the second screenshot above.
  2. Verified that the width and height are changed to 150, and there is no is-resized class:

width-height-changed

  1. Clicked the 75% pillbox, as shown in the first two screenshots.
  2. Verified that the width and height are correct, and the is-resized class is present:

is-resized-class

Tested on Chrome Version 68.0.3440.106 (Official Build) (64-bit)

Screenshots

Before:
no-width-height

After:
width-height-present-media-library

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

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

Before, images only had a width or height
if the user edited the <input> elements for those,
or clicked a percentage pillbox.
This sets the width and height on adding an image,
and on selecting from the 'Image Size' select element.
It keeps the behavior of the  class:
This only applied on editing the width or height <input>,
or on clicking a percentage pillbox.
@kienstra kienstra changed the title Output width and height attributes for image blocks Output width and height attributes for Image blocks Aug 29, 2018
@kienstra
Copy link
Contributor Author

kienstra commented Aug 29, 2018

It looks like the Travis build failed, but that might not be caused by this PR's changes. Does someone have permissions to click 'Restart build'?

There was a failed build,
based on errors that might not have been related to this PR.
@Soean
Copy link
Member

Soean commented Aug 29, 2018

@kienstra I restarted the test. The test snapshots with image blocks have no height and width attributes like in your code, so they fail.

@kienstra
Copy link
Contributor Author

Thanks, @Soean! I'll work on that now.

The build failed,
with an error at test/e2e/specs/adding-blocks.test.js
So begin to address this.
There were conflicts in 4 files
Resolve those, mainly retaining the master branch changes.
According to the documentation,
you're not supposed to edit these files directly.
The e2e tests failed,
so update these.
There was a failing test for this,
and it doesn't seem meanignful to have "" values for these.
@kienstra
Copy link
Contributor Author

Thank You
Travis Builds Passing

Hi @Soean,
Thanks for your help in restarting the Travis build, and pointing to the solution to the failed tests.

Would you be able to use this PR for #6652? Sorry for the delay in fixing the tests.

@kienstra
Copy link
Contributor Author

Question About PR

Hi @joemcgill,
Hope you're doing great!

Could you use this PR for #6652, and if so, would you be available to review it?

Thanks!

- core__image__attachment-link.json
- core__image__center-caption.json
- core__image__custom-link.json
- core__image__media-link.json
@hacknug
Copy link

hacknug commented Nov 16, 2018

Any plans on merging this any time soon?

@kienstra
Copy link
Contributor Author

If you would like to merge this, please ping me here and I'll resolve the merge conflicts.

@AdrienPerinot
Copy link

@kienstra I see this was closed. It seems to me that it's a rather important enhancement, as most if not all of the lazyload plugins rely on this to avoid jumps. There seem to be no clean workaround at the moment. Why was it closed :( ? Can it be planned for a future update ?

As a side note, inline images should also have these height/width attributes.

@kienstra
Copy link
Contributor Author

Hi @AdrienPerinot,
Thanks for bringing this up.

I closed it because it wasn't reviewed. But if someone would like to review it, I'd be glad to update it and resolve the merge conflicts.

@AdrienPerinot
Copy link

I see - wish it was reviewed as it is a must have in my opinion.

@kienstra
Copy link
Contributor Author

Thanks! Maybe contacting core Gutenberg contributor(s) would help.

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.

4 participants