[RNMobile] Extract caption component #16825

mchowning · 2019-07-30T21:04:21Z

Description

This extracts out a component for captions to be used by image and video blocks. Its use in image blocks should cause no behavior change. Its use in video blocks is a change from the current plain text caption. As a result:

Video blocks now have rich-text captions (gutenberg-mobile issue); and
The "Virtual keyboard dismisses when focus moves from image/video caption to paragraph" issue is resolved for video captions (image captions were addressed in a previous PR).

This also adds the ability to select the video block when the video block's caption is selected without playing the video. This is important so that you can bring back the video block's edit buttons in the toolbar after selecting the caption, and I confirmed with @iamthomasbishop that this was desired behavior.

How has this been tested?

The behavior for the image block captions should be unchanged, and video blocks should now have rich text captions.

I tested this by following the testing scenarios from the original PR adding rich text captions to the image block on both image and video captions.

The only issue with testing this on video blocks is that because currently mobile video captions do not currently work on the web and web video captions do not work on mobile, I was not able to really test the video captions interop with web. The html for the captions looks good though. Interestingly, although the video block from mobile doesn't display the video itself on web, I was able to see the captions for the (undisplayed) videos on web. 🤷‍♂️

One additional scenario I tested is the selection of the video block from the caption:

Select the caption for a video block
Observe rich text editing buttons in the toolbar without the pencil editing icon for the video block
Tap on the video above the caption
Observe that:
The rich text editing buttons are removed from the toolbar;
The pencil icon for editing the video block is added to the toolbar;
The video does not start playing; and
The keyboard is dismissed

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.

etoledom · 2019-08-01T08:33:50Z

packages/components/src/caption/index.native.js

@@ -0,0 +1,40 @@
+/**


If I understand correctly, this file seems to be a duplicate, right?
Since we are using the component from the block-editor package, this one seems to not be necessary 🤔

Would've sworn I removed that. Thanks for catching it.

It's removed now.

mchowning · 2019-08-01T20:42:08Z

packages/block-editor/src/components/caption/index.native.js

+				placeholder={ __( 'Write caption…' ) }
+				value={ text }
+				onChange={ onChange }
+				unstableOnFocus={ onFocus }


Because Caption's onFocus prop is being passed on to an unstable RichText prop (unstableOnFocus), perhaps I should also name this prop for caption unstableOnFocus instead of just onFocus like it is currently?

I think is fine? If unstableOnFocus changes, I guess we can modify the behavior internally to keep the caption.onFocus stable.

This move avoids a dependency issue that was causing the mobile tests to fail due to the Caption component's import of RichText from the block-editor package.

etoledom

Tested on iOS and Android and it works great on both 🎉

The original issue "Soft keyboard dismissal on Android" is also fixed 👍
I left a code comment but I don't think it's really a blocker.

Great job!

etoledom · 2019-08-08T16:53:27Z

packages/block-editor/src/components/caption/index.native.js

+import { RichText } from '@wordpress/block-editor';
+import { __ } from '@wordpress/i18n';
+
+const Caption = ( { accessible, accessibilityLabel, onBlur, onChange, onFocus, isParentSelected, isSelected, text } ) => {


isParentSelected
It feels "weird" the need to know about the parent's state, but I'm not sure if it's a bad practice or not.
What do you think about passing a hidden prop instead (or similar)?

cc @Tug what do you think?

I'd say it depends, usually we'd want to use an isVisible props in children to avoid unmounting a component from the dom or similar things, it's not a very common thing though.
In this case I think we could do without. just have `{ isSelected && <Caption ... /> }' would make the code simpler (less dependencies).

It feels "weird" the need to know about the parent's state, but I'm not sure if it's a bad practice or not.
What do you think about passing a hidden prop instead (or similar)?

It does feel a bit weird, I agree. I think the weirdness (for me at least) just comes from explicitly referencing the parent in the prop name. If I understand correctly, you're suggesting we could pass down a prop specifying whether the child should be hidden? My concern with that was that I wanted to pull the logic for whether the child should be hidden (line 14) into this component instead of having it separately implemented in any block that contains captions. Obviously, it's just one line, so it wouldn't be a big deal to pull it up into the (currently two) components that use captions, but I liked this solution better myself.

UPDATE: Actually, with @Tug 's suggestion of connecting the Caption component to the store, I was able to remove this prop (although Caption is still relying on the parent block's selected state, it's just getting that state from the store now). Let me know what you think.

In this case I think we could do without. just have `{ isSelected && <Caption ... /> }' would make the code simpler (less dependencies).

That is similar to how we used to hide the caption:

{ ( ! RichText.isEmpty( caption ) > 0 || isSelected ) && ( <Caption .../> ) }

but I needed to change that in an earlier PR to use the display style property to show/hide the caption in order to avoid a keyboard dismissal bug on Android.

Tug · 2019-08-08T17:26:38Z

packages/block-editor/src/components/caption/index.native.js

+			<RichText
+				rootTagsToEliminate={ [ 'p' ] }
+				placeholder={ __( 'Write caption…' ) }
+				value={ text }


Since we're in the block editor package, I think we could easily "connect" this component to the store to get/update the caption attribute. It could use a clientId as a props instead and we would be removing onChange and text.

That's a really interesting idea @Tug ! Thanks for sharing it! I'm not sure I know how to implement your suggestion 😁, but I took a shot at it and I have updated this PR. All those changes are in a single commit, so it might be easiest to just take a look at that to get a feel for what I did. I think I like this a lot better. The one concern I have is that the Caption component now depends on being passed a clientId that will get it a an attribute object with a caption property. I think this is ok, but I don't love it. What do you think about that and about this approach in general? No worries if this isn't what you had in mind or is a bad idea for some reason. I can easily revert the change.

This is a pretty big change, so I definitely think if we want to keep it this PR should be retested.

👋 @etoledom : if you want to give this PR another look, I'd be interested in hearing any thoughts you might have!

Tug · 2019-08-08T17:26:55Z

packages/block-editor/src/components/caption/index.native.js

+				value={ text }
+				onChange={ onChange }
+				unstableOnFocus={ onFocus }
+				onBlur={ onBlur } // always assign onBlur as props


Seems like onBlur is never passed to Caption

Good eye! 🕵️‍♂️ I have updated this.

I'm curious, do you know why we "always assign onBlur as props"? I see this comment all over the place, but I never knew why. Looks like it may have to do with bubbling blur events back up from the native side?

Also, I was surprised that missing this didn't seem to cause any problems. I did a bit of digging and it looks like neither image nor video have an onBlur prop themselves, so it makes sense that passing nothing was no worse in this case than passing undefined.

Tug

Code looks good overall 👍
Feel free to merge as it since it has already been accepted

etoledom

Thank you @mchowning for the updates!
Great improvement. Thanks @Tug too for the recommendations 🙏

Tested again on iOS and Android

* Extract caption component * Use caption component for video * Move Caption to block-editor for mobile tests This move avoids a dependency issue that was causing the mobile tests to fail due to the Caption component's import of RichText from the block-editor package. * Pass onBlur to Caption component * Connect Caption component to store

mchowning mentioned this pull request Jul 30, 2019

Extract caption component wordpress-mobile/gutenberg-mobile#1254

Merged

mchowning self-assigned this Jul 31, 2019

mchowning requested review from etoledom, daniloercoli and Tug July 31, 2019 21:16

mchowning added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Jul 31, 2019

etoledom reviewed Aug 1, 2019

View reviewed changes

mchowning force-pushed the rnmobile/extract_caption_component branch from 7f487d3 to 2e5beca Compare August 1, 2019 10:20

mchowning commented Aug 1, 2019

View reviewed changes

mchowning added 3 commits August 8, 2019 08:30

Extract caption component

91a3be7

Use caption component for video

f319621

Move Caption to block-editor for mobile tests

a7f40c9

This move avoids a dependency issue that was causing the mobile tests to fail due to the Caption component's import of RichText from the block-editor package.

mchowning force-pushed the rnmobile/extract_caption_component branch from 2e5beca to a7f40c9 Compare August 8, 2019 12:39

etoledom approved these changes Aug 8, 2019

View reviewed changes

Tug reviewed Aug 8, 2019

View reviewed changes

Pass onBlur to Caption component

51c7687

mchowning force-pushed the rnmobile/extract_caption_component branch from 28a1144 to c674586 Compare August 8, 2019 23:43

Connect Caption component to store

064b88e

mchowning force-pushed the rnmobile/extract_caption_component branch from c674586 to 064b88e Compare August 9, 2019 00:03

mchowning requested a review from etoledom August 9, 2019 00:11

etoledom approved these changes Aug 16, 2019

View reviewed changes

mchowning merged commit 15f5ff4 into master Aug 16, 2019

mchowning deleted the rnmobile/extract_caption_component branch August 16, 2019 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Extract caption component #16825

[RNMobile] Extract caption component #16825

mchowning commented Jul 30, 2019 •

edited

Loading

etoledom Aug 1, 2019

mchowning Aug 1, 2019 •

edited

Loading

mchowning Aug 1, 2019 •

edited

Loading

etoledom Aug 8, 2019

etoledom left a comment •

edited

Loading

etoledom Aug 8, 2019

Tug Aug 8, 2019

mchowning Aug 8, 2019 •

edited

Loading

Tug Aug 8, 2019

mchowning Aug 8, 2019 •

edited

Loading

Tug Aug 8, 2019

mchowning Aug 8, 2019 •

edited

Loading

Tug left a comment

etoledom left a comment

[RNMobile] Extract caption component #16825

[RNMobile] Extract caption component #16825

Conversation

mchowning commented Jul 30, 2019 • edited Loading

Description

How has this been tested?

Checklist:

etoledom Aug 1, 2019

Choose a reason for hiding this comment

mchowning Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

mchowning Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

etoledom Aug 8, 2019

Choose a reason for hiding this comment

etoledom left a comment • edited Loading

Choose a reason for hiding this comment

etoledom Aug 8, 2019

Choose a reason for hiding this comment

Tug Aug 8, 2019

Choose a reason for hiding this comment

mchowning Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Tug Aug 8, 2019

Choose a reason for hiding this comment

mchowning Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Tug Aug 8, 2019

Choose a reason for hiding this comment

mchowning Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Tug left a comment

Choose a reason for hiding this comment

etoledom left a comment

Choose a reason for hiding this comment

mchowning commented Jul 30, 2019 •

edited

Loading

mchowning Aug 1, 2019 •

edited

Loading

mchowning Aug 1, 2019 •

edited

Loading

etoledom left a comment •

edited

Loading

mchowning Aug 8, 2019 •

edited

Loading

mchowning Aug 8, 2019 •

edited

Loading

mchowning Aug 8, 2019 •

edited

Loading