Skip to content

Media & Text Block: Add image fill option#14445

Merged
frontdevde merged 14 commits intoWordPress:masterfrom
frontdevde:update/media-text-block-image-fill
Apr 3, 2019
Merged

Media & Text Block: Add image fill option#14445
frontdevde merged 14 commits intoWordPress:masterfrom
frontdevde:update/media-text-block-image-fill

Conversation

@frontdevde
Copy link
Copy Markdown
Contributor

@frontdevde frontdevde commented Mar 14, 2019 •
edited
Loading

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
frontdevde
frontdevde commented Mar 14, 2019
View reviewed changes
packages/block-library/src/media-text/deprecated.js Outdated
Copy link
Copy Markdown
Contributor Author

@frontdevde frontdevde Mar 14, 2019

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
Copy Markdown

michelleweber commented Mar 14, 2019

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
Closed
5 tasks
@kjellr
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

frontdevde commented Mar 21, 2019

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

@frontdevde frontdevde mentioned this pull request Mar 21, 2019
Merged
11 tasks
m-e-h
m-e-h reviewed Mar 21, 2019
View reviewed changes
getdave
getdave reviewed Mar 25, 2019
View reviewed changes
Copy link
Copy Markdown
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...

jorgefilipecosta
jorgefilipecosta reviewed Mar 27, 2019
View reviewed changes
Copy link
Copy Markdown
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 👍

@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
Copy Markdown
Contributor Author

frontdevde commented Apr 3, 2019

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
Copy Markdown
Contributor Author

frontdevde commented Apr 3, 2019

@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
Copy Markdown
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
Copy Markdown
Member

m-e-h commented Apr 3, 2019 •
edited
Loading

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
Copy Markdown
Contributor Author

frontdevde commented Apr 3, 2019

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.

jorgefilipecosta
jorgefilipecosta approved these changes Apr 3, 2019
View reviewed changes
Copy link
Copy Markdown
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
This was referenced Apr 15, 2019
Merged
Closed
Closed
Merged
@koke koke mentioned this pull request Oct 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@getdave getdave getdave left review comments

@m-e-h m-e-h m-e-h left review comments

@jorgefilipecosta jorgefilipecosta jorgefilipecosta approved these changes

@marekhrabe marekhrabe Awaiting requested review from marekhrabe

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@gziolo gziolo Awaiting requested review from gziolo

@noisysocks noisysocks Awaiting requested review from noisysocks

@notnownikki notnownikki Awaiting requested review from notnownikki

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

@aduth aduth Awaiting requested review from aduth

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

Assignees

@frontdevde frontdevde

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

Milestone

5.5 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

9 participants

@frontdevde @michelleweber @kjellr @dianeco @m-e-h @mapk @getdave @jorgefilipecosta @youknowriad