-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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] Block title in unsupported block #18268
Changes from 12 commits
e7d8e59
a23f7a3
766ce90
756b5e8
3a9cf57
47b491b
056b1bd
28b6a08
40ec6da
eab0ba3
6bb7dcb
e5a2433
5a9e429
4c2b904
f7c8582
32d4887
4627420
6815b3c
83bba75
91e74a9
b2bbd73
e863087
92d7e71
c4bc0c8
d150383
69998f0
e26a61b
0569a38
973c2f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { View, Text } from 'react-native'; | ||
import { Platform, View, Text, TouchableOpacity, TouchableWithoutFeedback } from 'react-native'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { Icon } from '@wordpress/components'; | ||
import { BottomSheet, Icon } from '@wordpress/components'; | ||
import { withPreferredColorScheme } from '@wordpress/compose'; | ||
import { coreBlocks } from '@wordpress/block-library'; | ||
import { normalizeIconObject } from '@wordpress/blocks'; | ||
|
@@ -18,26 +18,86 @@ import { __ } from '@wordpress/i18n'; | |
*/ | ||
import styles from './style.scss'; | ||
|
||
const isAndroid = Platform.OS === 'android'; | ||
const platformText = isAndroid ? 'Android' : 'iOS'; | ||
export class UnsupportedBlockEdit extends Component { | ||
constructor( props ) { | ||
super( props ); | ||
this.state = { showHelp: false }; | ||
} | ||
|
||
toggleSheet() { | ||
this.setState( { | ||
showHelp: ! this.state.showHelp, | ||
} ); | ||
} | ||
|
||
renderHelpIcon() { | ||
return <TouchableWithoutFeedback | ||
accessibilityLabel={ __( 'Help icon' ) } | ||
accessibilityRole={ 'button' } | ||
accessibilityHint={ __( 'Tap here to show help' ) } | ||
onPress={ this.toggleSheet.bind( this ) } | ||
> | ||
<View style={ styles.helpIconContainer } > | ||
<Icon | ||
className="unsupported-icon-help" | ||
label={ __( 'Help icon' ) } | ||
icon="editor-help" | ||
/> | ||
</View> | ||
</TouchableWithoutFeedback>; | ||
} | ||
|
||
renderSheetButton( text, action ) { | ||
return <TouchableOpacity | ||
onPress={ action() } | ||
style={ styles.sheetButton } | ||
> | ||
<Text style={ styles.sheetButtonText } >{ text }</Text> | ||
</TouchableOpacity>; | ||
} | ||
|
||
renderSheet( title ) { | ||
return <BottomSheet | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in f7c8582. |
||
isVisible={ this.state.showHelp } | ||
hideHeader | ||
onClose={ this.toggleSheet.bind( this ) } | ||
> | ||
<View style={ styles.infoContainer } > | ||
<Icon icon="editor-help" style={ styles.infoIcon } size={ styles.infoIcon.size } /> | ||
<Text style={ [ styles.infoText, styles.infoTitle ] }> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed with 5a9e429. |
||
{ __( '\'' + title + '\' isn\'t yet supported on WordPress for ' + platformText ) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can't compose strings inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed with 4c2b904, 6815b3c. |
||
</Text> | ||
<Text style={ [ styles.infoText, styles.infoDescription ] }> | ||
{ __( 'We are working hard to add more blocks with each release. In the meantime, you can also edit this post on the web.' ) } | ||
</Text> | ||
{ this.renderSheetButton( __( 'Close' ), () => this.toggleSheet.bind( this ) ) } | ||
</View> | ||
</BottomSheet>; | ||
} | ||
|
||
render() { | ||
const { originalName } = this.props.attributes; | ||
const { getStylesFromColorScheme, preferredColorScheme } = this.props; | ||
const blockType = coreBlocks[ originalName ]; | ||
|
||
const title = blockType ? blockType.settings.title : __( 'Unsupported' ); | ||
const title = blockType ? blockType.settings.title : originalName; | ||
const titleStyle = getStylesFromColorScheme( styles.unsupportedBlockMessage, styles.unsupportedBlockMessageDark ); | ||
|
||
const subTitleStyle = getStylesFromColorScheme( styles.unsupportedBlockSubtitle, styles.unsupportedBlockSubtitleDark ); | ||
const subtitle = blockType ? <Text style={ subTitleStyle }>{ __( 'Unsupported' ) }</Text> : null; | ||
const subtitle = <Text style={ subTitleStyle }>{ __( 'Unsupported' ) }</Text>; | ||
|
||
const icon = blockType ? normalizeIconObject( blockType.settings.icon ) : 'admin-plugins'; | ||
const iconStyle = getStylesFromColorScheme( styles.unsupportedBlockIcon, styles.unsupportedBlockIconDark ); | ||
const iconClassName = 'unsupported-icon' + '-' + preferredColorScheme; | ||
return ( | ||
<View style={ getStylesFromColorScheme( styles.unsupportedBlock, styles.unsupportedBlockDark ) }> | ||
{ this.renderHelpIcon() } | ||
<Icon className={ iconClassName } icon={ icon && icon.src ? icon.src : icon } color={ iconStyle.color } /> | ||
<Text style={ titleStyle }>{ title }</Text> | ||
{ subtitle } | ||
{ this.renderSheet( title ) } | ||
</View> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,72 @@ | ||
/** @format */ | ||
.content { | ||
padding-top: 8; | ||
padding-bottom: 0; | ||
padding-left: 24; | ||
padding-right: 24; | ||
align-items: center; | ||
justify-content: space-evenly; | ||
} | ||
|
||
.helpIconContainer { | ||
position: absolute; | ||
top: 0; | ||
right: 0; | ||
height: 48; | ||
width: 48; | ||
padding-top: 12; | ||
padding-right: 12; | ||
justify-content: flex-start; | ||
align-items: flex-end; | ||
} | ||
|
||
.infoContainer { | ||
flex-direction: column; | ||
align-items: center; | ||
justify-content: flex-end; | ||
} | ||
|
||
.infoIcon { | ||
size: 36; | ||
height: 36; | ||
padding-top: 8; | ||
padding-bottom: 8; | ||
} | ||
|
||
.infoText { | ||
text-align: center; | ||
} | ||
|
||
.infoTitle { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we must create a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed with 5a9e429. |
||
padding-top: 8; | ||
padding-bottom: 12; | ||
font-size: 20; | ||
font-weight: bold; | ||
color: $gray-dark; | ||
} | ||
|
||
.infoDescription { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed with 5a9e429. |
||
padding-bottom: 24; | ||
font-size: 16; | ||
color: $gray-darken-20; | ||
} | ||
|
||
.sheetButton { | ||
border-top-width: 1px; | ||
border-color: #f0f0f0; | ||
height: 48; | ||
width: 100%; | ||
justify-content: center; | ||
} | ||
|
||
.sheetButtonText { | ||
font-size: 16; | ||
text-align: center; | ||
color: #2271b1; | ||
} | ||
|
||
.unsupportedBlock { | ||
background-color: #e9eff3; // grey lighten 30 | ||
background-color: $gray-lighten-30; | ||
padding-top: 24; | ||
padding-bottom: 24; | ||
padding-left: 8; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Missing block renders without crashing 1`] = ` | ||
<View> | ||
<View | ||
accessibilityHint="Tap here to show help" | ||
accessibilityLabel="Help icon" | ||
accessibilityRole="button" | ||
accessible={true} | ||
clickable={true} | ||
onClick={[Function]} | ||
onResponderGrant={[Function]} | ||
onResponderMove={[Function]} | ||
onResponderRelease={[Function]} | ||
onResponderTerminate={[Function]} | ||
onResponderTerminationRequest={[Function]} | ||
onStartShouldSetResponder={[Function]} | ||
> | ||
Svg | ||
</View> | ||
Svg | ||
<Text> | ||
missing/block/title | ||
</Text> | ||
<Text> | ||
Unsupported | ||
</Text> | ||
Modal | ||
</View> | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import renderer from 'react-test-renderer'; | ||
import { Text } from 'react-native'; | ||
jest.mock( 'Platform', () => { | ||
const Platform = require.requireActual( 'Platform' ); | ||
Platform.OS = 'ios'; | ||
return Platform; | ||
} ); | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { BottomSheet, Icon } from '@wordpress/components'; | ||
jest.mock( '@wordpress/blocks' ); | ||
jest.mock( '../styles.scss', () => { | ||
return { | ||
infoIcon: { size: 32 }, | ||
unsupportedBlockIcon: { color: 'white' }, | ||
}; | ||
} ); | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import UnsupportedBlockEdit from '../edit.native.js'; | ||
|
||
const defaultAttributes = { | ||
originalName: 'missing/block/title', | ||
}; | ||
|
||
const getTestComponentWithContent = ( attributes = defaultAttributes ) => { | ||
return renderer.create( <UnsupportedBlockEdit attributes={ attributes } /> ); | ||
}; | ||
|
||
describe( 'Missing block', () => { | ||
it( 'renders without crashing', () => { | ||
const component = getTestComponentWithContent(); | ||
const rendered = component.toJSON(); | ||
expect( rendered ).toMatchSnapshot(); | ||
} ); | ||
|
||
describe( 'help modal', () => { | ||
it( 'renders help icon', () => { | ||
const component = getTestComponentWithContent(); | ||
const testInstance = component.root; | ||
const icons = testInstance.findAllByType( Icon ); | ||
expect( icons.length ).toBe( 2 ); | ||
expect( icons[ 0 ].props.icon ).toBe( 'editor-help' ); | ||
} ); | ||
|
||
it( 'renders info icon on modal', () => { | ||
const component = getTestComponentWithContent(); | ||
const testInstance = component.root; | ||
const bottomSheet = testInstance.findByType( BottomSheet ); | ||
const children = bottomSheet.props.children.props.children; | ||
expect( children.length ).toBe( 4 ); // 4 children in the bottom sheet: the icon, the "isn't yet supported" title, the "We are working hard..." message and the "Close" button | ||
expect( children[ 0 ].props.icon ).toBe( 'editor-help' ); | ||
} ); | ||
|
||
it( 'renders unsupported text on modal', () => { | ||
const component = getTestComponentWithContent(); | ||
const testInstance = component.root; | ||
const bottomSheet = testInstance.findByType( BottomSheet ); | ||
const children = bottomSheet.props.children.props.children; | ||
expect( children[ 1 ].props.children ).toBe( '\'' + defaultAttributes.originalName + '\' isn\'t yet supported on WordPress for iOS' ); | ||
} ); | ||
|
||
it( 'renders close button on modal', () => { | ||
const component = getTestComponentWithContent(); | ||
const testInstance = component.root; | ||
const bottomSheet = testInstance.findByType( BottomSheet ); | ||
const children = bottomSheet.props.children.props.children; | ||
expect( children[ 3 ].props.children.props.children ).toBe( 'Close' ); | ||
} ); | ||
} ); | ||
|
||
it( 'renders admin plugins icon', () => { | ||
const component = getTestComponentWithContent(); | ||
const testInstance = component.root; | ||
const icons = testInstance.findAllByType( Icon ); | ||
expect( icons.length ).toBe( 2 ); | ||
expect( icons[ 1 ].props.icon ).toBe( 'admin-plugins' ); | ||
} ); | ||
|
||
it( 'renders title text without crashing', () => { | ||
const component = getTestComponentWithContent(); | ||
const testInstance = component.root; | ||
const texts = testInstance.findAllByType( Text ); | ||
expect( texts.length ).toBe( 2 ); | ||
expect( texts[ 0 ].props.children ).toBe( 'missing/block/title' ); | ||
expect( texts[ 1 ].props.children ).toBe( 'Unsupported' ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,4 +75,7 @@ module.exports = { | |
unsupportedBlockIcon: { | ||
color: 'white', | ||
}, | ||
infoIcon: { | ||
size: 36, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also parentheses missing here. Also FYI, suggested this as a lint rule in #18302
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed with e863087.