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

Fix Media & Text “crop image to fill” to work with linked media #27211

Merged

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Nov 23, 2020

Issues

Using a Media & Text block with both linked media and the ”Crop image to fill entire column” results in a styling failure. The img element is not hidden as intended (to allow the “crop” to be of the background image).

Editor: Frontend:

Variations of the breakage can be seen in the following issues: #21399; https://core.trac.wordpress.org/ticket/49890

Additionally, I noticed that if the media is switched to a video while the ”Crop image to fill entire column” option is on then the Focal Point Picker is left in the UI which is possibly confusing as it has no effect on video media.

A screen recording to demonstrate leftover focal point picker

Changes

To address the issues

  • Change the selector for the img element to be descendent instead of child combinator.
  • Add a ruleset for the a to make the link cover the media area since the image is hidden.
  • Omit the ”Focal Point Picker” when the media is not an image

Tangential changes

  • Remove the use of figure in the selectors for the media element

How has this been tested?

  1. Insert a Media & Text block
  2. Add an image
  3. Add a link
  4. Verify ”Crop image to fill entire column” setting works as intended (frontend)
  5. Remove the link
  6. Repeat step No.4
  7. Change the media to a video
  8. Verify that the Focal Point Picker is omitted from the UI

Types of changes

Bug fix: #21399

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@stokesman stokesman force-pushed the fix/media-text-crop-fill-plus-link branch from 0fd9348 to 5ce9099 Compare November 23, 2020 20:04
@stokesman stokesman marked this pull request as ready for review November 23, 2020 21:01
@stokesman stokesman force-pushed the fix/media-text-crop-fill-plus-link branch from 5ce9099 to 111b012 Compare December 1, 2020 23:25
@stokesman
Copy link
Contributor Author

As best I can tell this has been an issue since #18139 was merged. It added support for linking media and it was overlooked that the added a element required some style adjustments (for the case where option “crop image to fill entire column” is enabled).

As they were involved there I'm asking if @draganescu, @mapk or @jorgefilipecosta may review this. Thank you 🙇

@skorasaurus skorasaurus added the [Block] Media & Text Affects the Media & Text Block label Dec 6, 2020
@draganescu
Copy link
Contributor

Howdy @stokesman and thanks for fixing this. Tested in every way I could think and it works swell! :shipit:

@draganescu draganescu merged commit e5b9df1 into WordPress:master Dec 7, 2020
@github-actions github-actions bot added this to the Gutenberg 9.6 milestone Dec 7, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with Media &Text Block
3 participants