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.

Sign up for GitHub

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

Jump to bottom

Update Image Block's image classes with dimensions #15464

Merged
merged 13 commits into from
Jun 18, 2019
Merged

Update Image Block's image classes with dimensions #15464

merged 13 commits into from
Jun 18, 2019

Conversation

draganescu
Copy link
Contributor

@draganescu draganescu commented May 6, 2019 •
edited
Loading

Description

Adds a class to the figure generated by the image block which corresponds to the slug of the image size selected in the inspector.

Screenshot 2019-05-07 at 18 37 30

How has this been tested?

  • insert an image block
  • add an image
  • from the inspector pick another size than the default
  • save the post
  • check the front end post result and inpect the image: the container figure element should have a class named size-{yourchosensize}.

Types of changes

  • updated the select control to allow for data attributes to be sent to it
  • updated the image block to save both in the editor and in the content the image size class

@draganescu draganescu mentioned this pull request May 6, 2019
Closed
@draganescu draganescu marked this pull request as ready for review May 7, 2019 10:40
@mapk
Copy link
Contributor

mapk commented May 15, 2019

I just tried testing the three sizes; full size, medium, and thumbnail and they all showed similarly named classes on the front-end. Looks good! 👍

@draganescu draganescu force-pushed the update/image-classes-with-dimensions branch from efc0db8 to 293b1e3 Compare May 28, 2019 08:36
@swissspidy swissspidy added the [Block] Image Affects the Image Block label May 28, 2019
@draganescu
Copy link
Contributor Author

somehow the size is undefined in the tests!

@gziolo
Copy link
Member

gziolo commented May 29, 2019

somehow the size is undefined in the tests!

Example:
https://travis-ci.com/WordPress/gutenberg/jobs/203472259#L1038

+ <figure class="wp-block-image size-undefined"><img alt=""/></figure>

@draganescu draganescu force-pushed the update/image-classes-with-dimensions branch from 293b1e3 to 17f45ee Compare May 31, 2019 14:47
noisysocks
noisysocks requested changes Jun 3, 2019
View reviewed changes
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

It works when I test it locally!! 🎉

I think that we can simplify the code a little in a few places, though—see comments.

packages/block-library/src/image/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/image/save.js Outdated Show resolved Hide resolved
packages/block-library/src/image/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/image/edit.js Outdated Show resolved Hide resolved
packages/e2e-tests/specs/__snapshots__/adding-blocks.test.js.snap Outdated Show resolved Hide resolved
@draganescu draganescu force-pushed the update/image-classes-with-dimensions branch from 0cb5c7b to ce195ab Compare June 4, 2019 18:53
@draganescu
Copy link
Contributor Author

I have refactored the whole thing according to @noisysocks 's review. Should be indeed simpler to follow now.

One thing this does not cover is what happens to images created before this. I have tested and until you change the size they do not get an updated classname. To have them get a class name I would need to:

  • get the current classname via a query attribute
  • get the slug out of that class name and set it as an attribute

I am not sure it is worth/ right / in the scope to implement this behaviour. Let me know @gziolo @noisysocks Thanks!

@gziolo
Copy link
Member

gziolo commented Jun 5, 2019

One thing this does not cover is what happens to images created before this. I have tested and until you change the size they do not get an updated classname. To have them get a class name I would need to:

This probably helps to avoid seeing the warning about invalid block's markup. Otherwise, it would generate a different HTML than this stored in the post content. I don't know how important it is to have those class names added but you can experiment with deprecated entry which uses migrate function to fill in the missing sizeSlug attribute. However, it might be really difficult given that those media sizes are fetched from the server. Some existing examples for other blocks:

https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/quote/deprecated.js#L40-L49

https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/gallery/deprecated.js#L80-L90

Have you tried to sync sizeSlug when edit from the block mounts?

@noisysocks
Copy link
Member

One thing this does not cover is what happens to images created before this. I have tested and until you change the size they do not get an updated classname.

Do they receive the classname on the next post save? I'd say that's an acceptable trade-off.

noisysocks
noisysocks approved these changes Jun 6, 2019
View reviewed changes
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

:shipit: :shipit: :shipit:

@draganescu draganescu merged commit 7568e9c into master Jun 18, 2019
@youknowriad youknowriad deleted the update/image-classes-with-dimensions branch June 24, 2019 09:45
@youknowriad youknowriad added this to the Gutenberg 6.0 milestone Jun 24, 2019
youknowriad
youknowriad reviewed Jun 26, 2019
View reviewed changes
packages/block-library/src/image/edit.js
@@ -339,17 +353,8 @@ class ImageEdit extends Component {
}

getImageSizeOptions() {
const { imageSizes, image } = this.props;
return compact( map( imageSizes, ( { name, slug } ) => {
const sizeUrl = get( image, [ 'media_details', 'sizes', slug, 'source_url' ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why this was removed as well. I'm assuming sometimes, images can be uploaded before the image size being defined which causes the size to not be available for that particular image.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see that you moved the check to the "updateImage" function

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the user could get confused if using the "select" doesn't change anything. Not critical for the moment though.

This was referenced Jul 16, 2024
Open
Open
@OKEAMAH OKEAMAH mentioned this pull request Jul 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@gziolo gziolo gziolo left review comments

@noisysocks noisysocks noisysocks approved these changes

@aduth aduth Awaiting requested review from aduth

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@notnownikki notnownikki Awaiting requested review from notnownikki

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

Assignees
No one assigned
Labels
[Block] Image Affects the Image Block
Projects
None yet
Milestone
Gutenberg 6.0
Development

Successfully merging this pull request may close these issues.
7 participants
@draganescu @mapk @gziolo @noisysocks @youknowriad @aduth @swissspidy