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] Extract caption component #16825

Merged
merged 5 commits into from Aug 16, 2019

Conversation

@mchowning
Copy link
Contributor

commented Jul 30, 2019

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:

  1. Video blocks now have rich-text captions (gutenberg-mobile issue); and
  2. 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.

Screen Shot 2019-07-30 at 3 32 39 PM

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:

  1. Select the caption for a video block
  2. Observe rich text editing buttons in the toolbar without the pencil editing icon for the video block
  3. Tap on the video above the caption
  4. Observe that:
  5. The rich text editing buttons are removed from the toolbar;
  6. The pencil icon for editing the video block is added to the toolbar;
  7. The video does not start playing; and
  8. 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.
@@ -0,0 +1,40 @@
/**

This comment has been minimized.

Copy link
@etoledom

etoledom Aug 1, 2019

Contributor

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 🤔

This comment has been minimized.

Copy link
@mchowning

mchowning Aug 1, 2019

Author Contributor

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

It's removed now.

@mchowning mchowning force-pushed the rnmobile/extract_caption_component branch from 7f487d3 to 2e5beca Aug 1, 2019
placeholder={ __( 'Write caption…' ) }
value={ text }
onChange={ onChange }
unstableOnFocus={ onFocus }

This comment has been minimized.

Copy link
@mchowning

mchowning Aug 1, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@etoledom

etoledom Aug 8, 2019

Contributor

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

mchowning added 3 commits Jul 30, 2019
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 mchowning force-pushed the rnmobile/extract_caption_component branch from 2e5beca to a7f40c9 Aug 8, 2019
Copy link
Contributor

left a comment

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!

import { RichText } from '@wordpress/block-editor';
import { __ } from '@wordpress/i18n';

const Caption = ( { accessible, accessibilityLabel, onBlur, onChange, onFocus, isParentSelected, isSelected, text } ) => {

This comment has been minimized.

Copy link
@etoledom

etoledom Aug 8, 2019

Contributor

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?

This comment has been minimized.

Copy link
@Tug

Tug Aug 8, 2019

Contributor

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).

This comment has been minimized.

Copy link
@mchowning

mchowning Aug 8, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@Tug

Tug Aug 8, 2019

Contributor

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.

This comment has been minimized.

Copy link
@mchowning

mchowning Aug 8, 2019

Author Contributor

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!

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

This comment has been minimized.

Copy link
@Tug

Tug Aug 8, 2019

Contributor

Seems like onBlur is never passed to Caption

This comment has been minimized.

Copy link
@mchowning

mchowning Aug 8, 2019

Author Contributor

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.

Copy link
Contributor

left a comment

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

@mchowning mchowning force-pushed the rnmobile/extract_caption_component branch from 28a1144 to c674586 Aug 8, 2019
@mchowning mchowning force-pushed the rnmobile/extract_caption_component branch from c674586 to 064b88e Aug 9, 2019
@mchowning mchowning requested a review from etoledom Aug 9, 2019
Copy link
Contributor

left a comment

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

Tested again on iOS and Android

@mchowning mchowning merged commit 15f5ff4 into master Aug 16, 2019
1 of 7 checks passed
1 of 7 checks passed
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@mchowning mchowning deleted the rnmobile/extract_caption_component branch Aug 16, 2019
gziolo added a commit that referenced this pull request Aug 29, 2019
* 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
gziolo added a commit that referenced this pull request Aug 29, 2019
* 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
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.