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

[RNMobile] Add Cover Block media settings #25810

Merged
merged 124 commits into from
Feb 15, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 3, 2020

Description

Addresses wordpress-mobile/gutenberg-mobile#1741 by adding the ability to edit the cover block focal point and toggle fixed position backgrounds.

I opted to rely upon React Native's PanResponder for the drag handle in the focal point editor. I attempted to rely upon react-native-gesture-handler, but incompatibility with Android modals forced me to rely upon PanResponder.

To support applying a max-height constraint (instead of a fill-parent strategy that all other images currently use) to an unknown sized image, I expanded the API of the existing Image component to broadcast when the image sizing metadata was known. This allows avoiding a collapsed element while the image loads.

How has this been tested?

User Test Cases

User case 1

  • Open the app with metro running.
  • Add a Cover block.
  • Select a color.
  • Add a text.
  • Tap the settings icon for the block.
  • Expect: Add image or video option available.
  • Add an image for the block.
  • Expect: Media preview displayed with focal point hint and edit media button in top right corner. Two options shown: Edit focal point and Fixed background toggle.

User case 2

  • Open the app with metro running.
  • Add a Cover block.
  • Set an image for the block.
  • Tap Edit focal point.
  • Expect: Media rendered with default focal point position displayed by drag handle and X/Y sliders.
  • Tap and drag handle to different location and release.
  • Expect: Slider values are updated.
  • Tap and drag handle on X-Axis Position slider.
  • Expect: Drag handle position is updated.
  • Tap and drag handle on Y-Axis Position slider.
  • Expect: Drag handle position is updated.
  • Tap Cancel.
  • Expect: Focal point changes are not persisted.

User case 3

  • Open the app with metro running.
  • Add a Cover block.
  • Set an image for the block.
  • Tap Edit focal point.
  • Tap and drag handle to different locaiton and release.
  • Tap Apply.
  • Expect: Focal point changes are persisted.
  • Toggle Fixed backgorund.
  • Expect: Focal point is cleared.

Screenshots

UX Interaction
ios-focal-point.mov
android-focal-point.mov
iOS Screenshots
Edit cover block Edit focal point
ios-edit-cover-block ios-edit-focal-point
ios-edit-cover-block-dark ios-edit-focal-point-dark
Android Screenshots
Edit cover block Edit focal point
android-edit-cover-block android-edit-focal-point
android-edit-cover-block-dark android-edit-focal-point-dark

Types of changes

New feature (non-breaking change which adds functionality)

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.

@ghost ghost force-pushed the rnmobile/media-settings branch from 217218c to d86f12d Compare October 4, 2020 21:00
@geriux geriux added [Status] In Progress Tracking issues with work in progress Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Oct 5, 2020
@geriux geriux self-requested a review October 7, 2020 08:12
@ghost ghost force-pushed the rnmobile/media-settings branch from ac36705 to 658acf0 Compare October 7, 2020 11:55
Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

Hey @shadow351 👋 just gave this I quick look at the code and its going on the right direction 👍

Just left two minor comments but I'll do a further review when it's ready for review.

Thanks!

@ghost ghost force-pushed the rnmobile/media-settings branch from 17fcf08 to 8814cef Compare October 11, 2020 22:17
@geriux
Copy link
Member

geriux commented Oct 16, 2020

I duplicated an existing tooltip component for my needs rather than attempting to abstract and share it because (A) the project directory structure of when/how components are shared is unclear to me at this time and (B) the existing tooltip's overlay and relative positioning did not align my use case of attaching to an absolutely positioned drag handle.

I can help here, I'd really prefer to make the tooltip shared even if it's just the basic functionality of it.

Can you elaborate a bit more about the issues you had?

Thanks!

@ghost
Copy link
Author

ghost commented Oct 16, 2020

I duplicated an existing tooltip component for my needs rather than attempting to abstract and share it because (A) the project directory structure of when/how components are shared is unclear to me at this time and (B) the existing tooltip's overlay and relative positioning did not align my use case of attaching to an absolutely positioned drag handle.

I can help here, I'd really prefer to make the tooltip shared even if it's just the basic functionality of it.

Can you elaborate a bit more about the issues you had?

Thanks!

@geriux thanks for the help.

First, in which directory should I place this shared tooltip component if I intend to share it between the page templates and focal point picker?

Second, the page template UI doesn't appear to be accessible within the react-native-editor test project. Where would I find that UI so that I may test any changes I make to a shared tooltip? I successfully set up wordpress-mobile/WordPress-iOS and wordpress-mobile/WordPress-Android to access the UI.

Third, I noticed a few differences between the use cases that caused me to pause sharing the code. I welcome your thoughts and opinions on whether you believe it is still worthwhile to share the code.

Page Template Focal Point
Requires an overlay to fill the entire window, but do not dismiss if tooltip target (page template buttons) is tapped. Requires overlay to fill entire bottom sheet, and dismiss if tooltip target (focal point drag handle) is tapped.
Tooltip relatively (i.e. no absolute style applied) positions itself against a sibling. Tooltip absolutely positioned against a sibling. Here the tooltip needs to be within a shared parent as the sibling, but must itself be absolutely positioned to avoid modifying the parent size and related focal point coordinate math (i.e. toggling the tooltip's presence would change the math).
Left-aligned tooltip arrow. Center-aligned tooltip arrow.

Placing the current Tooltip into FocalPointGroup means the tooltip overlay cannot expand to cover the entire bottom sheet, because FocalPointGroup is absolutely positioned itself.

<BottomSheet>
  <FocalPointGroup> /* absolutely positioned based on focal point coordinates */
    <Tooltip /> /* overlay unable to "break out" of FocalPointGroup */
    <FocalPointIcon />
  </FocalPointGroup>
</BottomSheet>

Placing the current Tooltip outside of FocalPointGroup means the tooltip no longer knowns where it should place itself in relation to the focal point icon.

<BottomSheet>
  <Tooltip /> /* not within FocalPointGroup, so does not know coordinate position to match */
  <FocalPointGroup> /* absolutely positioned based on focal point coordinates */
    <FocalPointIcon />
  </FocalPointGroup>
</BottomSheet>

I have not fully explored the feasibility of these options, but here are the ideas I have thus far for sharing a tooltip between the two use cases.

Option 1

Expand the current Tooltip to allow dictating the placement coordinates to match the FocalPointGroup. We'd just have to allow a placement prop.

<BottomSheet>
  <Tooltip placement={{ x: focalPoint.x, y: focalPoint.y }} />
  <FocalPointGroup placement={{ x: focalPoint.x, y: focalPoint.y }}>
    <FocalPointIcon />
  </FocalPointGroup>
</BottomSheet>

Option 2

Rather than including the overlay within Tooltip, we could export a separate TooltipOverlay component so the parent component could dictate its dimensions/behavior. This could be a render prop or we could use React context.

<BottomSheet>
  <Tooltip.Overlay style={{ height: '100%', width: '100%' }}>
    {({ visible }) => (
      <FocalPointGroup placement={{ x: focalPoint.x, y: focalPoint.y }}>
        <Tooltip visible={visible} />
        <FocalPointIcon />
      </FocalPointGroup>
    )}
  </Tooltip.Overlay>
</BottomSheet>

A third option may be something simpler that shares the basic functionality, as you stated. I'd be curious to hear what you are picturing. Let me know what you think. Thanks!

@ghost
Copy link
Author

ghost commented Oct 18, 2020

@geriux I refactored my tooltip in this branch to match the expectations that tapping anywhere should dismiss the modal (per Thomas' request). I then explored a variation of the tooltip that would allow using it for both use cases — page templates and focal point picker. The exploration is still rough, but I can clean it up more if you believe it is worth pursuing.

My thoughts on it are the it definitely expands the tooltip API surface area and adds complexity. I am not sure if it provides meaningful value at this point with only two points of usage. Part of me wonders if we should wait to see if more use cases arise before settling on an API for a shared tooltip. What do you think?

@ghost ghost marked this pull request as ready for review October 19, 2020 02:26
@geriux geriux removed the [Status] In Progress Tracking issues with work in progress label Oct 19, 2020
@ghost ghost force-pushed the rnmobile/media-settings branch 2 times, most recently from 1833cef to a1c78d2 Compare October 25, 2020 18:34
@ghost ghost requested review from Soean and talldan as code owners October 28, 2020 02:01
@ghost ghost force-pushed the rnmobile/media-settings branch 2 times, most recently from d59db4c to 14b6b1d Compare October 30, 2020 19:17
Previously, it was impossible to select the edit image button, as it was
rendered beneath the image itself.
@dcalhoun
Copy link
Member

dcalhoun commented Feb 10, 2021

Edit menu/actions

Do you think tapping anywhere on the thumbnail cell should show the ActionSheet (example)? And if so, should the edit button be anchored to the top-right of the cell and not the image?

🟢 Touchable Cell: Yes, for both visual users and a11y, I think it makes sense for the entire cell to be touchable. Yes, moving the edit button to top-right of the cell would make more sense to me. I will implement that within this PR.

@iamthomasbishop I'm going to reverse course on a "Touchable Cell" that triggers the ActionSheet/menu. From building it out, I've discovered there is an issue in the bottom sheet that causes it to not appropriately disregard taps on buttons when the bottom sheet scrolls. I.e. it triggers a button tap even if you are swiping to scroll. To be clear, this issue appears to already exist for all buttons in the bottom sheet, but the large height/width of the new focal point preview cell exacerbates the issue as it is much easier to trigger this scenario.

I am happy to create a separate issue for the button sensitivity in the bottom sheet. How do you feel about postponing this "Touchable Cell" change and retain the smaller edit image button for the time being?

Video of bottom sheet sensitivity
erroneous-bottom-sheet-touch.mov

Additionally, here is an example of what Horizontal Overflow visibility for the focal point cross-hair looks like:

Horizontal Overflow example Image focal-point-horizontal-overflow

Also, from playing with Vertical Overflow visibility, it feels a bit awkward for the cross-hair to sit atop the button beneath. Aside from the side effects and testing concerns I mentioned for the bottom cell, this awkwardness may be a reason to avoid Vertical Overflow visibility. Thoughts?

Vertical Overflow example Image focal-point-horizontal-overflow

Align with image edit button found within the canvas.
This allows dismissing the bottom sheet _after_ the second picker,
rather than before it. This is helpful for the scenario where a user
cancels the first picker. Now the bottom sheet will still be open in
that scenario.
The related menu also provides the ability to clear the media.
Previously, voice over would announce nothing when selecting this
button, as it did not have a label or hint.
@iamthomasbishop
Copy link

Thanks @dcalhoun!

Height Transition

it would appear this regression is unrelated to my work

Sounds good, thank you for investigating!

Overflow

Would you be OK with postponing this for another Issue?

Additionally, here is an example of what Horizontal Overflow visibility for the focal point cross-hair looks like:

Fixing the horizontal overflow makes a big difference, I'm much less concerned about the vertical, so yes we can worry about that in a separate pass.

Adding it to the canvas edit button may be more challenging, as tapping it would require a double-navigation event

That makes sense, let's defer that for another time. Just thinking out loud, we could potentially just drop the user on the top-level sheet and show a hint of some sort (perhaps something like the little pulsing blue dots that we show for Quick Start after the site creation flow).

Clear media Menu: Related to options within the ActionSheet/menu, do we desire to include Clear Media as an option for the inspector controls edit button? That is currently an option for the canvas edit button.

I was thinking yes, in the spirit of consistency. While I tend to prefer not duplicating entry points if possible, if we're going to use the same edit menu component in multiple places, it would make sense for those actions to be consistent.

Tap Media Action

Given this topic spans multiple media type blocks, I would ask if you are OK addressing changes to this in a separate ticket?

Definitely, fine to defer on that as well.

I've discovered there is an issue in the bottom sheet that causes it to not appropriately disregard taps on buttons when the bottom sheet scrolls

Ahh, that's right, I had forgotten about that interaction issue. I'm fine with deferring that one to another day as well.

it feels a bit awkward for the cross-hair to sit atop the button beneath

I see what you mean. That's totally fine to let the button sit "over" the cross-hair, seems like a reasonable trade-off.


I think I covered all of your questions, but let me know if I missed or need to clarify anything 😄

@dcalhoun
Copy link
Member

@iamthomasbishop thank you. You have answered all my questions. I will move forward seeking final code review and merge this work. If you'd like for me to hold off to allow you to review another build or have further discussion, just let me know. 😃

@dcalhoun
Copy link
Member

@geriux I believe this is ready for another code review. All the sibling PRs have been updated, including installable builds. Please let me know if there is anything else I need to do. Thanks!

Avoid a static English string.
Address styling oddities for images and videos within the focal point
settings. Namely, Android would render videos with the incorrect width,
overflowing the parent container.
Rendering `react-native-video` with `paused` set to `true` causes the
video to not render visibly on Android only. This is presumably related
to other issues regarding `paused`. https://git.io/Jt6Dr

By seeking the video to 0, the video correctly renders on Android.
@dcalhoun
Copy link
Member

dcalhoun commented Feb 13, 2021

@geriux I have addressed the a11y string issue and the missing video within the inspector for Android. This should be ready for review again. New builds are available on the sibling branches. Thanks!

Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @dcalhoun! 🎉 👏

This was a complex task and I'm so happy it's finally ready to be merged! Thank you for all the changes.

Can you add this new feature to the release notes before merging? Thanks!

Tested on both iOS and Android.

@geriux geriux merged commit 6ef9e82 into WordPress:master Feb 15, 2021
@github-actions github-actions bot added this to the Gutenberg 10.1 milestone Feb 15, 2021
@dcalhoun dcalhoun deleted the rnmobile/media-settings branch February 25, 2021 22:48
@hypest
Copy link
Contributor

hypest commented Mar 8, 2021

👋 @dcalhoun , this might be a naive question: as I'm trying to understand the two different components named FocalPointSettings (one here and the other one here), can you elaborate when you get the chance, on what's the difference between the two, perhaps in their usage too, and whether we could maybe rename one or the other to differentiate?

@dcalhoun
Copy link
Member

dcalhoun commented Mar 8, 2021

@hypest the former is the button in the parent sheet used to navigate to the focal point picker. The latter is the focal point picker itself, which is the child sheet. I agree that more explicit naming would be helpful.

IIRC I attempted to inline the latter into its parent, but navigation would not work. I never identified why that was. It likely related to the absence of a React Navigation context. Hence, it was moved into a separate file and confusingly named.

Long-term, the ideal would be to refactor this work to leverage the new BottomSheetSubSheet introduced in #28543. That would move some of the complexity of child sheet to an abstracted location.

@hypest
Copy link
Contributor

hypest commented Mar 8, 2021

Aha, I see now, thanks for elaborating David!

With us still relying quite a bit on search-in-files for navigating the codebase, I think it would pay off if we try to do some renaming here, though not a high priority by any measure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants