[TEST] Media & Text: Allow media to be selected and render focal point

[geriux]

Gutenberg Mobile PR -> wordpress-mobile/gutenberg-mobile#2426
Gutenberg PR -> WordPress/gutenberg#22788
WordPress Android PR -> wordpress-mobile/WordPress-Android#12072

This is a PR to test a build only.

To test:

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

	Warnings
⚠️	This PR is tagged with 'DO NOT MERGE'.

Generated by 🚫 dangerJS

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 31136. IPA is available here. If you need access to this, you can ask a maintainer to add you.

[github-actions]

	Warnings
⚠️	This PR is labelled with 'DO NOT MERGE'. Please don't merge it.

Generated by 🚫 dangerJS against a860a87

[peril-wordpress-mobile]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[iamthomasbishop]

@geriux Would you mind creating another build for me? I'm not seeing one on here — maybe expired or change since the last one was made? Sorry for the delay!

[geriux]

Hey @iamthomasbishop don't worry, I've just made a new build =) Let me know, thanks!

[iamthomasbishop]

@geriux Thanks for the updated build. I spent some time testing and have some questions and feedback. To be clear, I'm under the assumption that because this isn't truly an "innerBlocks" situation, some of this might be difficult (or not possible) without a full switch to nested blocks — so my hope is that we can close that gap as much as possible so that it feels as close as possible to that structure.

Notes

• When the media is selected, would it be possible to give it the full "proper" selection UI? Ideally, the results of that would look like this (visual example):
  • The selection outline would be on the media block itself, not the parent M & T block
  • The inline toolbar would provide tools specific to the media block — horizontal movers, settings button, and overflow menu button
  • The floating toolbar would transform to represent the (nested) media block
• Note: If we were able to do the above ☝️, we could then access the block-specific settings as well — things like linking and captions for images and playback controls for videos, etc.. It would also provide more consistency.
• I think our setting for "crop image to fill entire column" isn't working properly. For example, if this is toggled on and if I have an image that is taller than the contents of the other side, the image should crop smaller so that it's the same height as the text side, right? From what I can tell, It currently works okay going the other way (larger text side forces the image to crop, but not vice versa).
• The result of the "crop image..." issue is that the focal point is not shown on our end, as far as I can tell. For example, if I set the focal point to top-left on web (screenshot), it doesn't appear to take on mobile (screenshot).

Other issues

• It looks like there is a small margin at the bottom of the image that isn't necessary, but this might bit be specific to the Media & Text block 🤔 (screenshot)
• I just realized we only allow Button (singular) block to be added — not Buttons (plural). This might just need to be updated since we recently introduced the Buttons block and essentially "deprecated" the Button. This also begs the question, should we open the Media & Text block to any blocks? I think this was supposed to be a temporary limitation.
• I'm not sure where this is coming from, but on iPad, some of the Popover menus are getting cut off oddly. It looks like the background fill is pushed halfway down. (screenshot)

[geriux]

@geriux Thanks for the updated build. I spent some time testing and have some questions and feedback.

Thank you so much for testing it and for the very detailed feedback =)

To be clear, I'm under the assumption that because this isn't truly an "innerBlocks" situation, some of this might be difficult (or not possible) without a full switch to nested blocks — so my hope is that we can close that gap as much as possible so that it feels as close as possible to that structure.

Right, before starting with this task I checked the status of the block and how to approach these changes since one of the ideas was to make it work as an InnerBlock. It is not as you said, a truly "InnerBlock". A change like this would need to be done for web as well and it is a bit of a more challenging task. I have no doubt this block would be changed eventually though.

• When the media is selected, would it be possible to give it the full "proper" selection UI? Ideally, the results of that would look like this (visual example):

  • The selection outline would be on the media block itself, not the parent M & T block
  • The inline toolbar would provide tools specific to the media block — horizontal movers, settings button, and overflow menu button
  • The floating toolbar would transform to represent the (nested) media block

• Note: If we were able to do the above ☝️, we could then access the block-specific settings as well — things like linking and captions for images and playback controls for videos, etc.. It would also provide more consistency.

If Media & Text worked with Innerblocks for the media part those items would kinda come out of the box with the implementation. Trying to fake that behavior though is a lot more complicated just to be added for some specific blocks like this one. If it's ok with you I'd say let's wait to see what plans are there for Media & Text, what do you think?

• I think our setting for "crop image to fill entire column" isn't working properly. For example, if this is toggled on and if I have an image that is taller than the contents of the other side, the image should crop smaller so that it's the same height as the text side, right? From what I can tell, It currently works okay going the other way (larger text side forces the image to crop, but not vice versa).
• The result of the "crop image..." issue is that the focal point is not shown on our end, as far as I can tell. For example, if I set the focal point to top-left on web (screenshot), it doesn't appear to take on mobile (screenshot).

Nice catch! This is definitely a bug 🐛 ! I'm preparing a new build with the fix.

Other issues

• It looks like there is a small margin at the bottom of the image that isn't necessary, but this might bit be specific to the Media & Text block 🤔 (screenshot)

I'll check it out.

• I just realized we only allow Button (singular) block to be added — not Buttons (plural). This might just need to be updated since we recently introduced the Buttons block and essentially "deprecated" the Button. This also begs the question, should we open the Media & Text block to any blocks? I think this was supposed to be a temporary limitation.

Oh yes, this was mentioned today, we are planning to remove this limit and make it available for all.

• I'm not sure where this is coming from, but on iPad, some of the Popover menus are getting cut off oddly. It looks like the background fill is pushed halfway down. (screenshot)

That looks odd, I'll check it out as well.

I'll let you know when another build is ready if you want to do another quick review. Thanks again Thomas!

[iamthomasbishop]

Trying to fake that behavior though is a lot more complicated just to be added for some specific blocks like this one. If it's ok with you I'd say let's wait to see what plans are there for Media & Text, what do you think?

That makes sense, considering. Let’s do what we can within these constraints and try to initiate some progress towards innerBlocks if there isn’t already one in progress.

Oh yes, this was mentioned today, we are planning to remove this limit and make it available for all.

Sounds good, thank you!

That looks odd, I'll check it out as well.

Just to clarify, I was on iOS 13.5.1 at the time, iirc.

Looking forward to more testing, thank you!

[geriux]

That makes sense, considering. Let’s do what we can within these constraints and try to initiate some progress towards innerBlocks if there isn’t already one in progress.

Related to this I just did a quick search and found this recent issue.

It looks like it won't happen anytime soon but at least there are some discussions about it.

[geriux]

Hey @iamthomasbishop 👋 there's a new build for you to test =)

It includes:

• Crop image to fill entire column issue
• Removes extra small margin at the bottom

I'm investigating the popover menu since I couldn't reproduce, it is not related to these changes but I'll still try to see if I can reproduce to create a new issue.

Let me know if you see anything else, thank you!

[geriux]

Hey @iamthomasbishop Sorry this PR got closed, I made a new build again with the changes mentioned above, let me know if you spot anything else. Thank you!