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] Fix crash bringing gutenberg master to mobile #17228

Merged
merged 9 commits into from
Sep 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ cache:
branches:
only:
- master
- rnmobile/master
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's do it if you plan to use rnmobile/master more often 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to revert that I'm not sure. I merely merged rnmobile/master in here (thus the amount of changes unrelated to the PR description) but it might need some work before we merge it back to master

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might actually need to branch off again as we focus on the Open Beta. Keeping it for now 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you will branch more often so it's fine to keep it here until we merge the mobile repo in here.


env:
global:
Expand Down
10 changes: 10 additions & 0 deletions packages/api-fetch/src/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

function apiFetch( options ) {
// eslint-disable-next-line no-console
console.warn( 'apiFetch called with options', options );
Copy link
Contributor

@mchowning mchowning Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of console.warn, what would you think about making this console.info or console.log so we don't add another yellow box warning to the debug apps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's pretty alarming if the app is trying to make an api request and we should be aware of it. At least it's as alarming as state update warning or deprecation notice imo.
What we could do if you think those warnings are disrupting the experience when testing on mobile is disable them completely. If we want to see those we can simply attach a debugger.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's pretty alarming if the app is trying to make an api request and we should be aware of it.

I'm seeing that warning every time I run the app. Is that expected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I see it as well, I haven't tracked down the issue yet, but we might have to when we tackle editing image options since it seems related to media.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining. If this is a we-need-to-fix-this-as-soon-as-reasonably-possible type of thing, I have no concern about leaving the warning if you want. Alternatively, removing the warning and filing an issue to add the warning back in (and fix whatever is causing it to fire) could make sense so this doesn't get forgotten.


// return a promise that never resolves
return new Promise( () => {} );
}

export default apiFetch;
1 change: 1 addition & 0 deletions packages/block-directory/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
},
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"dependencies": {
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/block-editor": "file:../block-editor",
Expand Down
6 changes: 6 additions & 0 deletions packages/block-editor/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## Master

### Deprecation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this change is doing here 🤔

Copy link
Contributor

@Tug Tug Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're fine, this is part of #17195

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this PR landed yesterday 👍


- `dropZoneUIOnly` prop in `MediaPlaceholder` component has been deprecated in favor of `disableMediaButtons` prop.

## 3.0.0 (2019-08-05)

### New Features
Expand Down
30 changes: 30 additions & 0 deletions packages/block-editor/src/components/block-icon/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* External dependencies
*/
import { get } from 'lodash';
import { View } from 'react-native';

/**
* WordPress dependencies
*/
import { Path, Icon, SVG } from '@wordpress/components';

export default function BlockIcon( { icon, showColors = false } ) {
if ( get( icon, [ 'src' ] ) === 'block-default' ) {
icon = {
src: <SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><Path d="M19 7h-1V5h-4v2h-4V5H6v2H5c-1.1 0-2 .9-2 2v10h18V9c0-1.1-.9-2-2-2zm0 10H5V9h14v8z" /></SVG>,
};
}

const renderedIcon = <Icon icon={ icon && icon.src ? icon.src : icon } />;
const style = showColors ? {
backgroundColor: icon && icon.background,
color: icon && icon.foreground,
} : {};

return (
<View style={ style }>
{ renderedIcon }
</View>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* External dependencies
*/
import { last } from 'lodash';

/**
* WordPress dependencies
*/
import { withSelect } from '@wordpress/data';
import { getDefaultBlockName } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import DefaultBlockAppender from '../default-block-appender';
import styles from './style.scss';

function BlockListAppender( {
blockClientIds,
rootClientId,
canInsertDefaultBlock,
isLocked,
renderAppender: CustomAppender,
} ) {
if ( isLocked ) {
return null;
}

if ( CustomAppender ) {
return (
<CustomAppender />
);
}

if ( canInsertDefaultBlock ) {
return (
<DefaultBlockAppender
rootClientId={ rootClientId }
lastBlockClientId={ last( blockClientIds ) }
containerStyle={ styles.blockListAppender }
placeholder={ blockClientIds.length > 0 ? '' : null }
/>
);
}

return null;
}

export default withSelect( ( select, { rootClientId } ) => {
const {
getBlockOrder,
canInsertBlockType,
getTemplateLock,
} = select( 'core/block-editor' );

return {
isLocked: !! getTemplateLock( rootClientId ),
blockClientIds: getBlockOrder( rootClientId ),
canInsertDefaultBlock: canInsertBlockType( getDefaultBlockName(), rootClientId ),
};
} )( BlockListAppender );
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

.blockListAppender {
padding-left: 16;
padding-right: 16;
padding-top: 12;
padding-bottom: 0; // will be flushed into inline toolbar height
border-color: transparent;
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class BlockListBlock extends Component {
>
{ isValid && this.getBlockForType() }
{ ! isValid &&
<BlockInvalidWarning blockTitle={ title } icon={ icon } />
<BlockInvalidWarning blockTitle={ title } icon={ icon } />
}
</View>
{ isSelected && <BlockMobileToolbar clientId={ clientId } /> }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@
}

.blockContainer {
background-color: $white;
padding-left: 16px;
padding-right: 16px;
padding-top: 12px;
padding-bottom: 12px;
}

.blockContainerFocused {
background-color: $white;
padding-left: 16px;
padding-right: 16px;
padding-top: 12px;
Expand Down
51 changes: 32 additions & 19 deletions packages/block-editor/src/components/block-list/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { Text, View, Platform, TouchableWithoutFeedback } from 'react-native';
import { Component } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { compose, withPreferredColorScheme } from '@wordpress/compose';
import { createBlock, isUnmodifiedDefaultBlock } from '@wordpress/blocks';
import { KeyboardAwareFlatList, ReadableContentView } from '@wordpress/components';

Expand All @@ -19,7 +19,7 @@ import { KeyboardAwareFlatList, ReadableContentView } from '@wordpress/component
*/
import styles from './style.scss';
import BlockListBlock from './block';
import DefaultBlockAppender from '../default-block-appender';
import BlockListAppender from '../block-list-appender';

const innerToolbarHeight = 44;

Expand Down Expand Up @@ -60,41 +60,47 @@ export class BlockList extends Component {
renderDefaultBlockAppender() {
return (
<ReadableContentView>
<DefaultBlockAppender
<BlockListAppender
rootClientId={ this.props.rootClientId }
containerStyle={ [
styles.blockContainerFocused,
this.blockHolderBorderStyle(),
{ borderColor: 'transparent' },
] }
renderAppender={ this.props.renderAppender }
/>
</ReadableContentView>
);
}

render() {
const { clearSelectedBlock, blockClientIds, isFullyBordered, title, header, withFooter = true, renderAppender } = this.props;

return (
<View
style={ { flex: 1 } }
onAccessibilityEscape={ this.props.clearSelectedBlock }
onAccessibilityEscape={ clearSelectedBlock }
>
<KeyboardAwareFlatList
{ ...( Platform.OS === 'android' ? { removeClippedSubviews: false } : {} ) } // Disable clipping on Android to fix focus losing. See https://github.com/wordpress-mobile/gutenberg-mobile/pull/741#issuecomment-472746541
accessibilityLabel="block-list"
autoScroll={ this.props.autoScroll }
innerRef={ this.scrollViewInnerRef }
extraScrollHeight={ innerToolbarHeight + 10 }
keyboardShouldPersistTaps="always"
style={ styles.list }
data={ this.props.blockClientIds }
extraData={ [ this.props.isFullyBordered ] }
data={ blockClientIds }
extraData={ [ isFullyBordered ] }
keyExtractor={ identity }
renderItem={ this.renderItem }
shouldPreventAutomaticScroll={ this.shouldFlatListPreventAutomaticScroll }
title={ this.props.title }
ListHeaderComponent={ this.props.header }
title={ title }
ListHeaderComponent={ header }
ListEmptyComponent={ this.renderDefaultBlockAppender }
ListFooterComponent={ this.renderBlockListFooter }
ListFooterComponent={ withFooter && this.renderBlockListFooter }
/>

{ renderAppender && blockClientIds.length > 0 &&
<BlockListAppender
rootClientId={ this.props.rootClientId }
renderAppender={ this.props.renderAppender }
/>
}
</View>
);
}
Expand All @@ -107,6 +113,7 @@ export class BlockList extends Component {
}

renderItem( { item: clientId, index } ) {
const blockHolderFocusedStyle = this.props.getStylesFromColorScheme( styles.blockHolderFocused, styles.blockHolderFocusedDark );
const { shouldShowBlockAtIndex, shouldShowInsertionPoint } = this.props;
return (
<ReadableContentView>
Expand All @@ -119,18 +126,20 @@ export class BlockList extends Component {
rootClientId={ this.props.rootClientId }
onCaretVerticalPositionChange={ this.onCaretVerticalPositionChange }
borderStyle={ this.blockHolderBorderStyle() }
focusedBorderColor={ styles.blockHolderFocused.borderColor }
focusedBorderColor={ blockHolderFocusedStyle.borderColor }
/> ) }
</ReadableContentView>
);
}

renderAddBlockSeparator() {
const lineStyle = this.props.getStylesFromColorScheme( styles.lineStyleAddHere, styles.lineStyleAddHereDark );
const labelStyle = this.props.getStylesFromColorScheme( styles.labelStyleAddHere, styles.labelStyleAddHereDark );
return (
<View style={ styles.containerStyleAddHere } >
<View style={ styles.lineStyleAddHere }></View>
<Text style={ styles.labelStyleAddHere } >{ __( 'ADD BLOCK HERE' ) }</Text>
<View style={ styles.lineStyleAddHere }></View>
<View style={ lineStyle }></View>
<Text style={ labelStyle } >{ __( 'ADD BLOCK HERE' ) }</Text>
<View style={ lineStyle }></View>
</View>
);
}
Expand All @@ -156,12 +165,15 @@ export default compose( [
getSelectedBlockClientId,
getBlockInsertionPoint,
isBlockInsertionPointVisible,
getSelectedBlock,
} = select( 'core/block-editor' );

const selectedBlockClientId = getSelectedBlockClientId();
const blockClientIds = getBlockOrder( rootClientId );
const insertionPoint = getBlockInsertionPoint();
const blockInsertionPointIsVisible = isBlockInsertionPointVisible();
const selectedBlock = getSelectedBlock();
const isSelectedGroup = selectedBlock && selectedBlock.name === 'core/group';
const shouldShowInsertionPoint = ( clientId ) => {
return (
blockInsertionPointIsVisible &&
Expand All @@ -173,7 +185,7 @@ export default compose( [
const selectedBlockIndex = getBlockIndex( selectedBlockClientId );
const shouldShowBlockAtIndex = ( index ) => {
const shouldHideBlockAtIndex = (
blockInsertionPointIsVisible &&
! isSelectedGroup && blockInsertionPointIsVisible &&
// if `index` === `insertionPoint.index`, then block is replaceable
index === insertionPoint.index &&
// only hide selected block
Expand Down Expand Up @@ -204,5 +216,6 @@ export default compose( [
replaceBlock,
};
} ),
withPreferredColorScheme,
] )( BlockList );

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* External dependencies
*/
import { Text, View } from 'react-native';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { withSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import styles from './style.scss';

const BlockInsertionPoint = ( { showInsertionPoint } ) => {
if ( ! showInsertionPoint ) {
return null;
}

return (
<View style={ styles.containerStyleAddHere } >
<View style={ styles.lineStyleAddHere }></View>
<Text style={ styles.labelStyleAddHere } >{ __( 'ADD BLOCK HERE' ) }</Text>
<View style={ styles.lineStyleAddHere }></View>
</View>
);
};

export default withSelect( ( select, { clientId, rootClientId } ) => {
const {
getBlockIndex,
getBlockInsertionPoint,
isBlockInsertionPointVisible,
} = select( 'core/block-editor' );
const blockIndex = getBlockIndex( clientId, rootClientId );
const insertionPoint = getBlockInsertionPoint();
const showInsertionPoint = (
isBlockInsertionPointVisible() &&
insertionPoint.index === blockIndex &&
insertionPoint.rootClientId === rootClientId
);

return { showInsertionPoint };
} )( BlockInsertionPoint );
14 changes: 12 additions & 2 deletions packages/block-editor/src/components/block-list/style.native.scss
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@
height: 2px;
}

.lineStyleAddHereDark {
background-color: $gray-50;
}

.labelStyleAddHere {
flex: 1;
text-align: center;
Expand All @@ -34,9 +38,12 @@
font-weight: bold;
}

.labelStyleAddHereDark {
color: $gray-20;
}

.containerStyleAddHere {
flex-direction: row;
background-color: $white;
}

.blockHolderSemiBordered {
Expand All @@ -54,7 +61,6 @@
}

.blockContainerFocused {
background-color: $white;
padding-left: 16;
padding-right: 16;
padding-top: 12;
Expand All @@ -65,6 +71,10 @@
border-color: $gray-lighten-30;
}

.blockHolderFocusedDark {
border-color: $gray-70;
}

.blockListFooter {
height: 80px;
}
Loading