[RNMobile] Refactor Caption component #19118

geriux · 2019-12-13T10:42:45Z

Gutenberg-mobile PR -> wordpress-mobile/gutenberg-mobile#1678

Description

Our current Caption component implementation was meant to be within a block level component, but now there are cases where we just want to use the Caption UI like in the Gallery block.

So this PR adds a new component BlockCaption that will have the current logic wrapping a Caption component which will only have the UI.

How has this been tested?

Open the app
Add an Image block
- Set a caption
- Expect: caption being set correctly
Add a Video block
- Set a caption
- Expect: caption being set correctly
Add a Gallery block
- Add some images
  - Set captions
  - Expect: captions being set correctly
- Set a caption to the Gallery
- Expect: caption being set correctly
Save post
Open it
- Expect: captions are showing correctly

Types of changes

Refactor

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.

…aption component

test-case-reminder

Here are some suggested test cases for this PR.

WordPress iOS:

Simultaneous uploads - steps
Image block - Insert image from device (failing) - steps
Image block - Insert image from device (cancel) - steps
Image block - Add Caption - steps
Image block - Close/Re-open post with an ongoing image upload - steps
Image block - Close post with an ongoing image upload - steps
Image block - Switch to classic editor with an image block in page - steps

WordPress Android:

Simultaneous uploads - steps
Image block - Insert image from device (failing) - steps
Image block - Insert image from device (cancel) - steps
Image block - Add Caption - steps
Image block - Close/Re-open post with an ongoing image upload - steps
Image block - Close post with an ongoing image upload - steps
Image block - Switch to classic editor with an image block in page - steps

WordPress iOS:

Simultaneous uploads - steps
Video block - Insert video from device (failing) - steps
Video block - Insert video from device (cancel) - steps
Video block - Add Caption - steps
Video block - Close/Re-open post with an ongoing video upload - steps
Video block - Close post with an ongoing video upload - steps
Video block - Switch to classic editor with a video block in page - steps

WordPress Android:

Simultaneous uploads - steps
Video block - Insert video from device (failing) - steps
Video block - Insert video from device (cancel) - steps
Video block - Add Caption - steps
Video block - Close/Re-open post with an ongoing video upload - steps
Video block - Close post with an ongoing video upload - steps
Video block - Switch to classic editor with a video block in page - steps

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration.

If you are a beginner in mobile platforms follow build instructions.

geriux · 2019-12-13T10:45:48Z

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

+const CAPTION_TAG_NAME = Platform.select( {
+	ios: 'p',
+	android: '',
+} );


This was in the Gallery block to make it work correctly on Android due to the mentioned regression so I moved it to the Caption component itself since it was affecting the rest of Blocks using it.

👍 Linking this up with the issue that was filed: wordpress-mobile/gutenberg-mobile#1651

I like that this platform divergence is consolidated into one place, OTOH, it seems we currently have to "choose" between two regressions for all captions: Enter key not working or First word not style-able. This PR does not introduce these issues, but we do lose some granularity in how we can "choose which regression" 🙈 for the different use cases. I'm not suggesting a change here, just noting, and hopefully those issues will be resolved, and this will soon be a moot point. 😄

Yeah you're right, by moving this to the Caption component it will affect Block level components with the First word not style-able regression, but it will allow users to add new lines on Android... I wonder what users use the most, styling in captions or entering new lines 😅

geriux · 2019-12-16T10:15:13Z

Completed some of the suggested test cases and all good ✅, skipped some not related to these changes.

mkevins

Nice work @geriux ! I've left some minor comments. Overall, this looks pretty good. I've tested this out on Pixel 3a - Android 10, and everything is working well, aside from the known issue with styling the first word of the caption (not introduced by this PR).

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

mkevins · 2019-12-17T06:40:52Z

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

+const CAPTION_TAG_NAME = Platform.select( {
+	ios: 'p',
+	android: '',
+} );


I like that this platform divergence is consolidated into one place, OTOH, it seems we currently have to "choose" between two regressions for all captions: Enter key not working or First word not style-able. This PR does not introduce these issues, but we do lose some granularity in how we can "choose which regression" 🙈 for the different use cases. I'm not suggesting a change here, just noting, and hopefully those issues will be resolved, and this will soon be a moot point. 😄

…/caption-block

…ssing accessibility label for the Video block's caption

geriux · 2019-12-18T18:39:39Z

Hi @mkevins 👋 I've updated the PR with the changes from your feedback 😃. I also added the accessibility label for the Video block's caption that was missing.

By the way, I saw that for the images within the gallery we are setting the accessibility label like this ->

gutenberg/packages/block-library/src/gallery/gallery-image.native.js

Line 266 in 319307c

    
           accessibilityLabel={ this.accessibilityLabelImageContainer() } // if we don't set this explicitly it reads system provided accessibilityLabels of all child components and those include pretty technical words which don't make sense

Would it make sense to move it to the Caption component? Now that it supports to pass the data through its props? What do you think?

Thanks!

mkevins · 2020-01-07T05:20:30Z

Hey @geriux 👋

I've tested again with the changes, and everything still works as before. Nice work!

By the way, I saw that for the images within the gallery we are setting the accessibility label like this ->

gutenberg/packages/block-library/src/gallery/gallery-image.native.js

Line 266 in 319307c

accessibilityLabel={ this.accessibilityLabelImageContainer() } // if we don't set this explicitly it reads system provided accessibilityLabels of all child components and those include pretty technical words which don't make sense

Would it make sense to move it to the Caption component? Now that it supports to pass the data through its props? What do you think?

I tried that on Android, and it seemed to be ok, but I haven't had a chance to check on iOS. Since the comment specifically mentions addressing a VoiceOver issue for iOS, perhaps it's best to leave it for now. The intent here is to treat an unselected GalleryImage as a "leaf", i.e. no children elements are talked out on the VoiceOver, so moving this to Caption may regress that.

mkevins

I had to locally merge with latest (gutenberg:master and gutenberg-mobile:develop) to the respective branches for these PRs due to an intermittent dependency issue, but after that, everything is working nicely. Nice work @geriux 👍 . After updating to latest, LGTM!

geriux · 2020-01-07T08:40:56Z

I had to locally merge with latest (gutenberg:master and gutenberg-mobile:develop) to the respective branches for these PRs due to an intermittent dependency issue, but after that, everything is working nicely. Nice work @geriux 👍 . After updating to latest, LGTM!

Awesome! Thanks for testing it!

geriux · 2020-01-08T08:18:39Z

Hey @pinarol ! This is ready to be merged but I want to double check this https://github.com/WordPress/gutenberg/pull/19118/files/865739561806ba1a735f161d40df17f935ddf00c#diff-fb4bb656dce8b784120b171a9b46cbcaR12-R17 first.

TL;DR With these changes we'll have one regression for captions (only on Android) which is that the first word can't be styled because we are not setting the CAPTION_TAG_NAME. The reason we are not setting it is because of another regression that doesn't allow adding new lines.

Currently, for (Android) we have two regressions in captions:

Gallery block caption User can add new lines but can't style first word of the caption
Other captions User can't add new lines but can style all words of the caption.

What do you think? Should we merge and have just one regression for all captions? Or wait until those bugs are fixed?

pinarol · 2020-01-09T08:59:28Z

@geriux So we need to choose between bugs :) I gave it a try and I think not being able to style feels more buggy than not being able to enter a new line. When I select the whole sentence and try to apply a style it is simply not responding. So I'd go with sacrificing the ability to add a new line. Plus, I don't expect multiple line captions are that common.

geriux · 2020-01-09T10:08:30Z

@geriux So we need to choose between bugs :) I gave it a try and I think not being able to style feels more buggy than not being able to enter a new line. When I select the whole sentence and try to apply a style it is simply not responding. So I'd go with sacrificing the ability to add a new line. Plus, I don't expect multiple line captions are that common.

Sounds good! I agree with multiple line captions not being that common, awesome! I'll update it then =)

Thank you!

…/caption-block

geriux · 2020-01-10T12:54:22Z

Hey @mkevins I changed this 1359e55 if it looks good to you I can go ahead and merge =)

mkevins · 2020-01-13T00:17:09Z

Hi @geriux 👋 This looks good to merge. Nice work!

Gerardo Pacheco added 2 commits December 12, 2019 16:36

RNMobile - Refactor of Caption component into two: BlockCaption and C…

e5af32d

…aption component

RNMobile - Refactor of Caption component

8657395

geriux 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 Dec 13, 2019

test-case-reminder bot reviewed Dec 13, 2019

View reviewed changes

geriux mentioned this pull request Dec 13, 2019

Refactor Caption Component wordpress-mobile/gutenberg-mobile#1678

Closed

1 task

geriux commented Dec 13, 2019

View reviewed changes

geriux marked this pull request as ready for review December 16, 2019 10:15

geriux requested a review from mkevins December 16, 2019 11:06

mkevins reviewed Dec 17, 2019

View reviewed changes

Gerardo Pacheco added 2 commits December 18, 2019 11:18

Merge branch 'master' of github.com:WordPress/gutenberg into refactor…

09d4700

…/caption-block

RNMobile - Caption refactor passing accessibility props and adding mi…

319307c

…ssing accessibility label for the Video block's caption

geriux requested a review from mkevins December 18, 2019 18:35

mkevins approved these changes Jan 7, 2020

View reviewed changes

geriux added the [Status] In Progress Tracking issues with work in progress label Jan 9, 2020

Gerardo Pacheco added 2 commits January 10, 2020 10:18

Merge branch 'master' of github.com:WordPress/gutenberg into refactor…

a7c1d9a

…/caption-block

RNMobile - Caption: Specify p tag for both platforms

1359e55

geriux removed the [Status] In Progress Tracking issues with work in progress label Jan 10, 2020

pinarol mentioned this pull request Jan 10, 2020

Gutenberg: update to v1.20.0 wordpress-mobile/WordPress-Android#11052

Merged

1 task

geriux merged commit 0f2ec15 into master Jan 13, 2020

geriux deleted the refactor/caption-block branch January 13, 2020 08:31

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

jbinda mentioned this pull request Feb 28, 2020

[RNMobile] Fix BlockCaption padding visibility #20535

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Refactor Caption component #19118

[RNMobile] Refactor Caption component #19118

geriux commented Dec 13, 2019 •

edited

Loading

test-case-reminder bot left a comment •

edited by geriux

Loading

geriux Dec 13, 2019

mchowning Dec 13, 2019

mkevins Dec 17, 2019

geriux Dec 18, 2019

geriux commented Dec 16, 2019

mkevins left a comment

mkevins Dec 17, 2019

geriux commented Dec 18, 2019

mkevins commented Jan 7, 2020

mkevins left a comment

geriux commented Jan 7, 2020

geriux commented Jan 8, 2020

pinarol commented Jan 9, 2020 •

edited

Loading

geriux commented Jan 9, 2020

geriux commented Jan 10, 2020

mkevins commented Jan 13, 2020

[RNMobile] Refactor Caption component #19118

[RNMobile] Refactor Caption component #19118

Conversation

geriux commented Dec 13, 2019 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

test-case-reminder bot left a comment • edited by geriux Loading

Choose a reason for hiding this comment

geriux Dec 13, 2019

Choose a reason for hiding this comment

mchowning Dec 13, 2019

Choose a reason for hiding this comment

mkevins Dec 17, 2019

Choose a reason for hiding this comment

geriux Dec 18, 2019

Choose a reason for hiding this comment

geriux commented Dec 16, 2019

mkevins left a comment

Choose a reason for hiding this comment

mkevins Dec 17, 2019

Choose a reason for hiding this comment

geriux commented Dec 18, 2019

mkevins commented Jan 7, 2020

mkevins left a comment

Choose a reason for hiding this comment

geriux commented Jan 7, 2020

geriux commented Jan 8, 2020

pinarol commented Jan 9, 2020 • edited Loading

geriux commented Jan 9, 2020

geriux commented Jan 10, 2020

mkevins commented Jan 13, 2020

geriux commented Dec 13, 2019 •

edited

Loading

test-case-reminder bot left a comment •

edited by geriux

Loading

pinarol commented Jan 9, 2020 •

edited

Loading