[RNMobile] Block title in unsupported block #18268

hypest · 2019-11-04T13:25:33Z

(this is a follow up from #17943 and will copy and amend its description here. Recreated that PR so we merge it while @illusaen is temporarily away)

Description

Changed text on Unsupported block to show original block name.
Added a help icon that shows additional information when the block is clicked.
Append "Unsupported" to the title of all unsupported blocks

How has this been tested?

Tested on simulator and on Android device.
Wrote unit tests for component change.
Component change affects all types of blocks that were defaulting to the 'Missing' block (aka UnsupportedBlockEdit)

Screenshots

Types of changes

New feature: Issue #479

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.

…ng original name in place of 'Unsupported'.

…ause the small icon was too hard to press. Also rearranging the BottomSheet to work the height paddings out correctly.

…or the help block. Few color changes. Adding some text to the help block.

maxme

Current implementation is off with dark mode for iOS, there are some changes needed in the style sheet and the block implementation (see review comments).

maxme · 2019-11-04T15:51:58Z

packages/block-library/src/missing/style.native.scss

+	text-align: center;
+}
+
+.infoTitle {


I think we must create a infoTitleDark class and update the color.

Addressed with 5a9e429.

maxme · 2019-11-04T15:52:02Z

packages/block-library/src/missing/style.native.scss

+	color: $gray-dark;
+}
+
+.infoDescription {


Same as for infoTitle, we should create a infoDescriptionDark class and update the color.

Addressed with 5a9e429.

maxme · 2019-11-04T15:52:47Z

packages/block-library/src/missing/edit.native.js

+		>
+			<View style={ styles.infoContainer } >
+				<Icon icon="editor-help" style={ styles.infoIcon } size={ styles.infoIcon.size } />
+				<Text style={ [ styles.infoText, styles.infoTitle ] }>


We should call getStylesFromColorScheme on there.

Addressed with 5a9e429.

koke · 2019-11-04T16:04:46Z

packages/block-library/src/missing/edit.native.js

+			<View style={ styles.infoContainer } >
+				<Icon icon="editor-help" style={ styles.infoIcon } size={ styles.infoIcon.size } />
+				<Text style={ [ styles.infoText, styles.infoTitle ] }>
+					{ __( '\'' + title + '\' isn\'t yet supported on WordPress for ' + platformText ) }


You can't compose strings inside __(), you need to use sprintf instead and add a placeholder for the title. For the platform, I think it's safer to have one string for iOS and one for Android, since it's more obvious for translators.

Addressed with 4c2b904, 6815b3c.

koke · 2019-11-04T16:10:06Z

packages/block-library/src/missing/edit.native.js

+	}
+
+	renderSheet( title ) {
+		return <BottomSheet


I'm surprised that it's not caught by the lint job, but I noticed a different JSX style in this file than the rest of the code: we normally wrap the returned JSX in parentheses and jump to a new line:

return ( <BottomSheet isVisible={ this.state.showHelp } hideHeader onClose={ this.toggleSheet.bind( this ) } > { /* ... */ } </BottomSheet> );

I can't find it mentioned in any coding style guide, but that's the style I've seen and written everywhere else.

Addressed in f7c8582.

hypest · 2019-11-05T10:28:41Z

👋 @maxme and @koke , thanks for the review! I've addressed your comments and this is ready for another pass.

I've also removed the "Close" button from the bottom sheet as we don't have such a dismiss button in other bottom sheets anyway. cc @iamthomasbishop, there was a relevant question in the original PR and I went with a "No" here... let me know if you think we should reinstate the Close button.

koke · 2019-11-06T10:26:18Z

packages/block-library/src/missing/edit.native.js

+	}
+
+	renderHelpIcon() {
+		return <TouchableWithoutFeedback


There are also parentheses missing here. Also FYI, suggested this as a lint rule in #18302

Addressed with e863087.

koke · 2019-11-06T10:28:25Z

packages/block-library/src/missing/edit.native.js

+
+		// translators: %s: Name of the block
+		const titleFormat = Platform.OS === 'android' ? __( '\'%s\' isn\'t yet supported on WordPress for Android' ) :
+			__( '\'%s\' isn\'t yet supported on WordPress for iOS' );


Not an issue for this PR, but noting for the future: I just realized this might be used in other apps, so we should find a way to inject the app name from the apps instead.

Good point!

hypest · 2019-11-06T16:37:43Z

Addressed the additional feedback @koke , ready for another pass.

maxme

I can't find color definition on the original issue (wordpress-mobile/gutenberg-mobile#479) or on the other PR (#17943). Anyway, the end results looks good to me. If the color scheme have to be changed, it can be done in a follow up PR.

iamthomasbishop · 2019-11-06T18:30:47Z

@maxme Looks good – the only color change I'd request is to change the (?) icons on the sheet and on the block itself to gray 20 (in dark mode).

hypest · 2019-11-07T08:35:28Z

the only color change I'd request is to change the (?) icons on the sheet and on the block itself to gray 20 (in dark mode).

Addressed with 92d7e71.

I set the light-mode icon color to gray-darken-20. Can you verify @iamthomasbishop that that's the light-mode color we want? Thanks!

Screenshots:

Light-mode list	Dark-mode list	Light-mode popup	Dark-mode popup

hypest · 2019-11-07T11:27:39Z

This PR already has a 👍 and checked in with @koke too and he's fine with the PR state. The colors should be OK now @iamthomasbishop, but if they're not we can open a new PR really quickly, no problem. I'll merge this one as soon as the gutenberg-mobile side PR also gets the 👍.

SergioEstevao

Working great on iOS (dark and light) and Android.

iamthomasbishop · 2019-11-07T14:56:31Z

@hypest Oops, I meant that gray 20 should be applied only to dark mode. I realize now my comment wasn't the clearest, so apologies for that 😄 That initial "old gray" is fine on light mode.

hypest · 2019-11-07T15:51:31Z

Sorry for the confusion @iamthomasbishop , apparently I should have restarted the app inbetween mode switching for the proper colors to appear. Without changing the code, here are new screenshots taken:

Light-mode list	Dark-mode list	Light-mode popup	Dark-mode popup

hypest · 2019-11-07T16:12:14Z

After chatting with @iamthomasbishop over Slack, revised the light-mode color of the icon on the sheet to be gray. Screenshots:

Light-mode list	Dark-mode list	Light-mode popup	Dark-mode popup

hypest · 2019-11-07T17:43:26Z

👋 @SergioEstevao , I know you've already 👍 'd the PR but, I added 0569a38 to fix the mobile tests (were failing on the gutenberg-mobile PR after e26a61b landed) and 973c2f8 to remove the hardcoded platform set and enable testing both platform so, can you have another pass please? Thanks!

SergioEstevao · 2019-11-07T21:54:52Z

@SergioEstevao I had another look and all still looking good!

illusaen and others added 8 commits October 14, 2019 18:14

WC: #479: Adding help icon and modal to unsupported block. Also putti…

e7d8e59

…ng original name in place of 'Unsupported'.

WC: #479: Adding in unit tests for the component.

a23f7a3

WC: #479: Changing the help icon press surface to the whole block bec…

766ce90

…ause the small icon was too hard to press. Also rearranging the BottomSheet to work the height paddings out correctly.

WC: #479: Changing accessibility label and adding comment.

756b5e8

WC: #479: Adding in text/buttons to match later design.

3a9cf57

WC: issue #479: Changing only the question mark icon to be tappable f…

47b491b

…or the help block. Few color changes. Adding some text to the help block.

Merge branch 'master' into rnmobile/block-title-in-unsupported-block

056b1bd

Display 'unsupported' on all unsupported blocks on mobile

28b6a08

hypest requested a review from SergioEstevao November 4, 2019 13:25

hypest requested review from ajitbohra, jorgefilipecosta, Soean and talldan as code owners November 4, 2019 13:25

hypest mentioned this pull request Nov 4, 2019

Changing Unsupported title and adding help info block (Issue #479) #17943

Closed

5 tasks

hypest removed request for talldan, Soean, ajitbohra and jorgefilipecosta November 4, 2019 13:26

hypest 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 Nov 4, 2019

hypest added this to the Gutenberg 6.9 milestone Nov 4, 2019

hypest mentioned this pull request Nov 4, 2019

Include the block title in unsupported blocks wordpress-mobile/gutenberg-mobile#1533

Merged

1 task

hypest added 4 commits November 4, 2019 16:40

Update the test snapshot of Unsupported block on mobile

40ec6da

Need to mock styling on mobile

eab0ba3

Adjust tests to latest changes

6bb7dcb

Display the block name in quotes, for clarity

e5a2433

maxme requested changes Nov 4, 2019

View reviewed changes

koke reviewed Nov 4, 2019

View reviewed changes

hypest added 3 commits November 5, 2019 10:41

Dark mode of info text, title and description

5a9e429

sprintf to compose string, separate strings per platform

4c2b904

Wrap JSX in the grouping operator, as usual

f7c8582

hypest added 3 commits November 5, 2019 11:29

Remove dismiss button, not used in other bottom-sheets

32d4887

Remove unused import

4627420

Revise sprintf format to green the linter

6815b3c

hypest added 2 commits November 5, 2019 12:31

Update tests after removing the Close button

83bba75

Merge branch 'master' into rnmobile/block-title-in-unsupported-block

91e74a9

koke reviewed Nov 6, 2019

View reviewed changes

hypest added 2 commits November 6, 2019 18:14

Merge branch 'master' into rnmobile/block-title-in-unsupported-block

b2bbd73

Wrap more JSX in the grouping operator, as usual

e863087

maxme approved these changes Nov 6, 2019

View reviewed changes

Adjust the info-icon color

92d7e71

hypest added 2 commits November 7, 2019 11:01

Merge branch 'master' into rnmobile/block-title-in-unsupported-block

c4bc0c8

Merge branch 'master' into rnmobile/block-title-in-unsupported-block

d150383

Merge branch 'master' into rnmobile/block-title-in-unsupported-block

69998f0

SergioEstevao approved these changes Nov 7, 2019

View reviewed changes

Different colors for the icon on the sheet

e26a61b

hypest added 2 commits November 7, 2019 19:38

Remove local mock, using the global styleMock.js instead

0569a38

Enable testing both mobile platforms

973c2f8

hypest merged commit 79636ce into master Nov 8, 2019

hypest deleted the rnmobile/block-title-in-unsupported-block branch November 8, 2019 07:40

hypest mentioned this pull request Nov 11, 2019

[RNMobile] Native mobile release v1.17.0 #18426

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Block title in unsupported block #18268

[RNMobile] Block title in unsupported block #18268

hypest commented Nov 4, 2019

maxme left a comment

maxme Nov 4, 2019

hypest Nov 5, 2019

maxme Nov 4, 2019

hypest Nov 5, 2019

maxme Nov 4, 2019

hypest Nov 5, 2019

koke Nov 4, 2019

hypest Nov 5, 2019 •

edited

koke Nov 4, 2019

hypest Nov 5, 2019

hypest commented Nov 5, 2019

koke Nov 6, 2019

hypest Nov 6, 2019

koke Nov 6, 2019

hypest Nov 6, 2019

hypest commented Nov 6, 2019

maxme left a comment

iamthomasbishop commented Nov 6, 2019 •

edited

hypest commented Nov 7, 2019

hypest commented Nov 7, 2019

SergioEstevao left a comment

iamthomasbishop commented Nov 7, 2019

hypest commented Nov 7, 2019

hypest commented Nov 7, 2019

hypest commented Nov 7, 2019 •

edited

SergioEstevao commented Nov 7, 2019

[RNMobile] Block title in unsupported block #18268

[RNMobile] Block title in unsupported block #18268

Conversation

hypest commented Nov 4, 2019

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

maxme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Nov 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest commented Nov 6, 2019

maxme left a comment

Choose a reason for hiding this comment

iamthomasbishop commented Nov 6, 2019 • edited

hypest commented Nov 7, 2019

hypest commented Nov 7, 2019

SergioEstevao left a comment

Choose a reason for hiding this comment

iamthomasbishop commented Nov 7, 2019

hypest commented Nov 7, 2019

hypest commented Nov 7, 2019

hypest commented Nov 7, 2019 • edited

SergioEstevao commented Nov 7, 2019

hypest Nov 5, 2019 •

edited

iamthomasbishop commented Nov 6, 2019 •

edited

hypest commented Nov 7, 2019 •

edited