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

Media & Text Block: Add image fill option #14445

Merged

Conversation

frontdevde
Copy link
Contributor

@frontdevde frontdevde commented Mar 14, 2019

Description

This PR aims to add an image fill option as an enhancement to the Media & Text block.

In the Media & Text block it would be nice to have the option to make the image fill the full height of its column. In addition to providing a new creative option, this would allow for a more consistent look of the block across different viewports.

The corresponding issue is #14226

Screenshots

image-fill

Todo

  • Copy review Let image fill the entire column

How has this been tested?

  • Manually verify ability to set image fill option
  • Manually verify ability to select focal point
  • Manually test how image fill behaves in Responsive (ie: non-desktop) context
  • Manually verify can preview image fill via block editor in editor mode
  • Manually verify save output on website aligns correctly in various permutations of image fill
  • Manually verify blocks created with older version of the block still work

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.

@frontdevde frontdevde added [Type] Enhancement A suggestion for improvement. Needs Copy Review Needs review of user-facing copy (language, phrasing) [Block] Media & Text Affects the Media & Text Block labels Mar 14, 2019
@frontdevde frontdevde self-assigned this Mar 14, 2019
@@ -0,0 +1,193 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the deprecated part to its own file to declutter index.js.

@michelleweber
Copy link

Looks fine!

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Mar 15, 2019
@frontdevde frontdevde removed the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Mar 15, 2019
@sarahmonster sarahmonster mentioned this pull request Mar 18, 2019
5 tasks
@kjellr
Copy link
Contributor

kjellr commented Mar 19, 2019

This seems pretty solid in my testing, and I think it's a welcome addition to the block. I do wonder if we should rephrase the label though.

Let image fill the entire column

This can be a little confusing because if there isn't much text in the "Text" column (or if the image is very tall), the image will already be filling the entire column. I wonder if mentioning "cropping" in some way would be more clear:

  • Automatically crop image and fill available space
  • Crop image to fill entire column

@frontdevde frontdevde force-pushed the update/media-text-block-image-fill branch from ce109ac to f24e634 Compare March 21, 2019 04:35
@frontdevde
Copy link
Contributor Author

Thanks for the feedback, @kjellr. As per your suggestion, I updated the copy to "Crop image to fill entire column".

Screen Shot 2019-03-20 at 23 46 13

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Great work here. Tested and it seems to work really well.

One a11y point which I think we should consider...

packages/block-library/src/media-text/media-container.js Outdated Show resolved Hide resolved
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Thank you for working on @frontdevde, it is looking good 👍

packages/block-library/src/media-text/index.js Outdated Show resolved Hide resolved
packages/block-library/src/media-text/media-container.js Outdated Show resolved Hide resolved
@frontdevde frontdevde added Needs Accessibility Feedback Need input from accessibility and removed Needs Accessibility Feedback Need input from accessibility labels Mar 28, 2019
@frontdevde frontdevde force-pushed the update/media-text-block-image-fill branch from 13ddb6d to 153fe18 Compare March 28, 2019 23:18
@frontdevde frontdevde force-pushed the update/media-text-block-image-fill branch from 6286e4a to 4aea210 Compare April 3, 2019 00:48
@frontdevde
Copy link
Contributor Author

Please note that with the merge of #13989 into master, I had to resolve a couple of conflicts and force push the rebased branch. In addition, I also updated the changelog to reflect the changes in this PR.

@frontdevde
Copy link
Contributor Author

@mapk Thank you for your input!

In light of those three points, it seems reasonable to move forward without object-fit for now.

Personally, I agree.

Can we stay consistent here for now and present the object-fit in another PR that can address this situation in the other blocks as well?

@jorgefilipecosta @m-e-h Does this work for you?

@getdave
Copy link
Contributor

getdave commented Apr 3, 2019

Using the background-image isn't ideal, but consistent with current code in the Cover block

A key point.

it seems reasonable to move forward without object-fit for now.

I'd agree with this too. I concur with many folk here that object-fit is the preferable solution but it would Block this PR and would require cover to be updated as well. Why not ship this nice enhancement to our users and then look to add the object-fit upgrade in a separate PR. This fits nicely with Matt's approach to such dilemas 😄

One question: I'm not clear why the background-image version wouldn't work in IE11.

Also, an Issue should be raised regarding object fit for both media-text and cover so that this isn't lost.

@m-e-h
Copy link
Member

m-e-h commented Apr 3, 2019

One question: I'm not clear why the background-image version wouldn't work in IE11.

initial used as the default position value isn't supported in IE.
https://developer.mozilla.org/en-US/docs/Web/CSS/initial#Browser_compatibility

Probably best to not insert background-position inline at all if it's not specified.

I'm sure IE would just ignore initial and use the initial value anyway. Just saying, with you guys being all IE this and IE that. 😄

@frontdevde
Copy link
Contributor Author

I've changed the default value for the background position from initial to 50% 50% now, which is supported by all browsers in our compatibility matrix and the intended result we want to see.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Things looked good on my tests, thank you for iterating on this 👍

@frontdevde frontdevde merged commit b3961d0 into WordPress:master Apr 3, 2019
@frontdevde frontdevde deleted the update/media-text-block-image-fill branch April 3, 2019 17:20
@youknowriad youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants