-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from 3 commits
05f7506
ad8d8d5
8fd916a
ea43258
c0312ff
74ee26f
5a7ee6d
d62ead0
df3a356
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 |
---|---|---|
|
@@ -20,6 +20,7 @@ cache: | |
branches: | ||
only: | ||
- master | ||
- rnmobile/master | ||
|
||
env: | ||
global: | ||
|
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 ); | ||
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. Instead of 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 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. 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. Disabled in wordpress-mobile/gutenberg-mobile@fb6db57 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 seeing that warning every time I run the app. Is that expected? 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. 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. 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. 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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,9 @@ | ||
## Master | ||
|
||
### Deprecation | ||
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. Not sure what this change is doing here 🤔 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. Looks like we're fine, this is part of #17195 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. 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 | ||
|
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 |
---|---|---|
@@ -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 ); |
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.
Yes, let's do it if you plan to use
rnmobile/master
more often 👍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.
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 masterThere 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.
We might actually need to branch off again as we focus on the Open Beta. Keeping it for now 👍
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.
I'm sure you will branch more often so it's fine to keep it here until we merge the mobile repo in here.