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] Simplify media insertion flow Part 1 - redux changes #29546

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6742b75
created actions for adding and clearing last inserted block event.
jd-alexander Mar 4, 2021
fc8d9b4
added reducer for determining new state based on the action
jd-alexander Mar 4, 2021
fb115ae
added selector to query the state for the last block inserted
jd-alexander Mar 4, 2021
e3fefb6
Merge branch 'trunk' into rnmobile/simplify_image_insertion_flow-redu…
jd-alexander Mar 16, 2021
ee4d502
[RNMobile] Simplify media insertion flow Part 2 - media upload (#29547)
jd-alexander Mar 16, 2021
cb31236
Added release notes for auto-opening.
jd-alexander Mar 17, 2021
3aa051f
Merge branch 'trunk' into rnmobile/simplify_image_insertion_flow-redu…
jd-alexander Mar 17, 2021
b12215f
Merge branch 'trunk' into rnmobile/simplify_image_insertion_flow-redu…
jd-alexander Mar 19, 2021
d865c2b
[RNMobile] Refactor simplify media flow redux store changes (#30123)
jd-alexander Mar 31, 2021
3654e91
Merge branch 'trunk' into rnmobile/simplify_image_insertion_flow-redu…
jd-alexander Mar 31, 2021
4135172
added wasBlockJustInserted prop needed after merge with trunk.
jd-alexander Mar 31, 2021
92ee6f0
Merge branch 'trunk' into rnmobile/simplify_image_insertion_flow-redu…
jd-alexander Apr 7, 2021
d22738f
Merge branch 'trunk' into rnmobile/simplify_image_insertion_flow-redu…
jd-alexander Apr 19, 2021
bb98f4c
removed updateSelection from reducer so it's updated at all times.
jd-alexander Apr 19, 2021
1941ae8
Merge branch 'trunk' into rnmobile/simplify_image_insertion_flow-redu…
jd-alexander Apr 20, 2021
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
8 changes: 7 additions & 1 deletion packages/block-editor/src/components/inserter/menu.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,13 @@ function InserterMenu( {
innerBlocks
);

insertBlock( newBlock, insertionIndex, destinationRootClientId );
insertBlock(
newBlock,
insertionIndex,
destinationRootClientId,
true,
{ source: 'inserter_menu' }
);
},
[ insertBlock, destinationRootClientId, insertionIndex ]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ This property is similar to the `accept` property. The difference is the format
- Required: No
- Platform: Web | Mobile

### autoOpenMediaUpload

If true, the MediaUpload component auto-opens the picker of the respective platform.

- Type: `Boolean`
- Required: No
- Default: `false`
- Platform: Mobile

### className

Class name added to the placeholder.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function MediaPlaceholder( props ) {
height,
backgroundColor,
hideContent,
autoOpenMediaUpload,
} = props;

// use ref to keep media array current for callbacks during rerenders
Expand Down Expand Up @@ -160,6 +161,7 @@ function MediaPlaceholder( props ) {
}
multiple={ multiple }
isReplacingMedia={ false }
autoOpen={ autoOpenMediaUpload }
render={ ( { open, getMediaOptions } ) => {
return (
<TouchableWithoutFeedback
Expand Down
9 changes: 9 additions & 0 deletions packages/block-editor/src/components/media-upload/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,15 @@ Only applies if `gallery === true`.
- Default: `false`
- Platform: Web

### autoOpen

If true, the picker of the respective platform auto-opens.

- Type: `Boolean`
- Required: No
- Default: `false`
- Platform: Mobile

### gallery

If true, the component will initiate all the states required to represent a gallery. By default, the media modal opens in the gallery edit frame, but that can be changed using the `addToGallery`flag.
Expand Down
32 changes: 28 additions & 4 deletions packages/block-editor/src/components/media-upload/index.native.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
/**
* External dependencies
*/
import { Platform } from 'react-native';

import { delay } from 'lodash';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { Component, React } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { Picker } from '@wordpress/components';
import {
Expand All @@ -26,20 +33,21 @@ export const OPTION_TAKE_VIDEO = __( 'Take a Video' );
export const OPTION_TAKE_PHOTO = __( 'Take a Photo' );
export const OPTION_TAKE_PHOTO_OR_VIDEO = __( 'Take a Photo or Video' );

const PICKER_OPENING_DELAY = 200;

export class MediaUpload extends Component {
constructor( props ) {
super( props );
this.onPickerPresent = this.onPickerPresent.bind( this );
this.onPickerSelect = this.onPickerSelect.bind( this );
this.getAllSources = this.getAllSources.bind( this );

this.state = {
otherMediaOptions: [],
};
}

componentDidMount() {
const { allowedTypes = [] } = this.props;
const { allowedTypes = [], autoOpen } = this.props;
getOtherMediaOptions( allowedTypes, ( otherMediaOptions ) => {
const otherMediaOptionsWithIcons = otherMediaOptions.map(
( option ) => {
Expand All @@ -54,6 +62,10 @@ export class MediaUpload extends Component {

this.setState( { otherMediaOptions: otherMediaOptionsWithIcons } );
} );

if ( autoOpen ) {
this.onPickerPresent();
}
}

getAllSources() {
Expand Down Expand Up @@ -136,8 +148,20 @@ export class MediaUpload extends Component {
}

onPickerPresent() {
const { autoOpen } = this.props;
const isIOS = Platform.OS === 'ios';

if ( this.picker ) {
this.picker.presentPicker();
// the delay below is required because on iOS this action sheet gets dismissed by the close event of the Inserter
// so this delay allows the Inserter to be closed fully before presenting action sheet.
if ( autoOpen && isIOS ) {
delay(
() => this.picker.presentPicker(),
PICKER_OPENING_DELAY
);
} else {
this.picker.presentPicker();
}
}
}

Expand Down
13 changes: 11 additions & 2 deletions packages/block-editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -538,16 +538,25 @@ export function* moveBlockToPosition(
* @param {?number} index Index at which block should be inserted.
* @param {?string} rootClientId Optional root client ID of block list on which to insert.
* @param {?boolean} updateSelection If true block selection will be updated. If false, block selection will not change. Defaults to true.
* @param {?Object} meta Optional Meta values to be passed to the action object.
*
* @return {Object} Action object.
*/
export function insertBlock(
block,
index,
rootClientId,
updateSelection = true
updateSelection = true,
meta
) {
return insertBlocks( [ block ], index, rootClientId, updateSelection );
return insertBlocks(
[ block ],
index,
rootClientId,
updateSelection,
0,
meta
);
}

/**
Expand Down
26 changes: 26 additions & 0 deletions packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1727,6 +1727,31 @@ export function highlightedBlock( state, action ) {
return state;
}

/**
* Reducer returning the block insertion event list state.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function lastBlockInserted( state = {}, action ) {
switch ( action.type ) {
case 'INSERT_BLOCKS':
if ( ! action.updateSelection || ! action.blocks.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

After a second thought, I think that we probably shouldn't include the ! action.updateSelection condition here. The last block inserted should be updated independently on if the block gets selected upon insertion, right?

Originally I thought that this was necessary because we only wanted to show the media picker when the block is selected, but I realised that we're already checking that in the components (example), what do you think?

Copy link
Contributor Author

@jd-alexander jd-alexander Apr 16, 2021

Choose a reason for hiding this comment

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

After a second thought, I think that we probably shouldn't include the ! action.updateSelection condition here. The last block inserted should be updated independently on if the block gets selected upon insertion, right?

when you say "updated independently on if the block gets selected upon insertion" what exactly do you mean? I want to ensure that I understand you clearly :)

Originally I thought that this was necessary because we only wanted to show the media picker when the block is selected, but I realised that we're already checking that in the components (example), what do you think?

Makes sense! I checked the linked example and it went to the withSelect in the Gallery component, so I am assuming that you are referring to the isSelected usage there. If that's the case, yes, the isSelected behavior was added before the redux approach was introduced, so if it's better to keep the selection logic within the reducer, I am not against that, especially if it will be consistent with other behavior in the reducer.

Another question, I wanted to add to the discussion. In which cases would the block be inserted and it wouldn't be a selection. I removed isSelected and updateSelection from the code in my local environs and the functionality still worked. I will look more into this tomorrow, but I just wanted to share my thoughts here, so you can add any ideas you may have.

Copy link
Contributor

Choose a reason for hiding this comment

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

when you say "updated independently on if the block gets selected upon insertion" what exactly do you mean? I want to ensure that I understand you clearly :)

Oh sorry if I wasn't too clear, what I meant is that if this slice of the state contains the last block inserted, it makes sense that it gets updated for any block that is inserted, including the ones with the param updateSelection as false. From a consistence POV, I would expect that if I insert a block even if it doesn't get automatically selected (via the updateSelection action param), it would be set as the last block inserted.

Makes sense! I checked the linked example and it went to the withSelect in the Gallery component, so I am assuming that you are referring to the isSelected usage there. If that's the case, yes, the isSelected behavior was added before the redux approach was introduced, so if it's better to keep the selection logic within the reducer, I am not against that, especially if it will be consistent with other behavior in the reducer.

Yeah exactly, I was referring to the isSelected usage when calculating the value to pass to autoOpenMediaUpload prop.

In my opinion I think it would make more sense to keep the isSelected usage as we have currently in the components and update the lastBlockInserted reducer.

Another question, I wanted to add to the discussion. In which cases would the block be inserted and it wouldn't be a selection. I removed isSelected and updateSelection from the code in my local environs and the functionality still worked. I will look more into this tomorrow, but I just wanted to share my thoughts here, so you can add any ideas you may have.

I think it's a uncommon case but looks like there's at least one place where it's being used in the Jetpack project:
https://github.com/Automattic/jetpack/blob/5b637a82a9836213f066809cab550ce3590921cb/projects/plugins/jetpack/extensions/blocks/anchor-fm/editor.js#L48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry if I wasn't too clear, what I meant is that if this slice of the state contains the last block inserted, it makes sense that it gets updated for any block that is inserted, including the ones with the param updateSelection as false. From a consistence POV, I would expect that if I insert a block even if it doesn't get automatically selected (via the updateSelection action param), it would be set as the last block inserted.

Ah I get what you are saying now. Thanks for the clarification 🙇🏾

In my opinion I think it would make more sense to keep the isSelected usage as we have currently in the components and update the lastBlockInserted reducer.

I agree with this as well after gaining a deeper understanding due to your clarifications above. I will make the relevant changes to support this.

I think it's a uncommon case but looks like there's at least one place where it's being used in the Jetpack project:
https://github.com/Automattic/jetpack/blob/5b637a82a9836213f066809cab550ce3590921cb/projects/plugins/jetpack/extensions/blocks/anchor-fm/editor.js#L48

Good find 😉 Thanks for checking Jetpack. If I had a sleuthing badge 🔍 I would definitely give you one 😄

Copy link
Contributor

@fluiddot fluiddot Apr 19, 2021

Choose a reason for hiding this comment

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

I agree with this as well after gaining a deeper understanding due to your clarifications above. I will make the relevant changes to support this.

Nice! Thanks for making the changes, I'll be watching them so we can have this PR ready as soon as possible.

Good find 😉 Thanks for checking Jetpack. If I had a sleuthing badge 🔍 I would definitely give you one 😄

Thank you very much! 🕵️😄

return state;
}

const clientId = action.blocks[ 0 ].clientId;
const source = action.meta?.source;

return { clientId, source };
case 'RESET_BLOCKS':
return {};
}
return state;
}

export default combineReducers( {
blocks,
isTyping,
Expand All @@ -1748,4 +1773,5 @@ export default combineReducers( {
hasBlockMovingClientId,
automaticChangeStatus,
highlightedBlock,
lastBlockInserted,
} );
16 changes: 16 additions & 0 deletions packages/block-editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -2131,3 +2131,19 @@ export const __experimentalGetActiveBlockIdByBlockNames = createSelector(
validBlockNames,
]
);

/**
* Tells if the block with the passed clientId was just inserted.
*
* @param {Object} state Global application state.
* @param {Object} clientId Client Id of the block.
* @param {?string} source Optional insertion source of the block.
* @return {boolean} True if the block matches the last block inserted from the specified source.
*/
export function wasBlockJustInserted( state, clientId, source ) {
const { lastBlockInserted } = state;
return (
lastBlockInserted.clientId === clientId &&
lastBlockInserted.source === source
);
}
99 changes: 99 additions & 0 deletions packages/block-editor/src/store/test/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
template,
blockListSettings,
lastBlockAttributesChange,
lastBlockInserted,
} from '../reducer';

describe( 'state', () => {
Expand Down Expand Up @@ -2944,4 +2945,102 @@ describe( 'state', () => {
expect( state ).toBe( null );
} );
} );

describe( 'lastBlockInserted', () => {
it( 'should return client id if last block inserted is called with action INSERT_BLOCKS', () => {
const expectedClientId = '62bfef6e-d5e9-43ba-b7f9-c77cf354141f';

const action = {
blocks: [
{
clientId: expectedClientId,
},
],
meta: {
source: 'inserter_menu',
},
type: 'INSERT_BLOCKS',
updateSelection: true,
};

const state = lastBlockInserted( {}, action );

expect( state.clientId ).toBe( expectedClientId );
} );

it( 'should return inserter_menu source if last block inserted is called with action INSERT_BLOCKS', () => {
const expectedSource = 'inserter_menu';

const action = {
blocks: [
{
clientId: '62bfef6e-d5e9-43ba-b7f9-c77cf354141f',
},
],
meta: {
source: expectedSource,
},
type: 'INSERT_BLOCKS',
updateSelection: true,
};

const state = lastBlockInserted( {}, action );

expect( state.source ).toBe( expectedSource );
} );

it( 'should return state if last block inserted is called with action INSERT_BLOCKS that is not a updateSelection', () => {
const expectedState = {
clientId: '9db792c6-a25a-495d-adbd-97d56a4c4189',
};

const action = {
blocks: [
{
clientId: '62bfef6e-d5e9-43ba-b7f9-c77cf354141f',
},
],
meta: {
source: 'inserter_menu',
},
type: 'INSERT_BLOCKS',
updateSelection: false,
};

const state = lastBlockInserted( expectedState, action );

expect( state ).toEqual( expectedState );
} );

it( 'should return state if last block inserted is called with action INSERT_BLOCKS and block list is empty', () => {
const expectedState = {
clientId: '9db792c6-a25a-495d-adbd-97d56a4c4189',
};

const action = {
blocks: [],
meta: {
source: 'inserter_menu',
},
type: 'INSERT_BLOCKS',
updateSelection: true,
};

const state = lastBlockInserted( expectedState, action );

expect( state ).toEqual( expectedState );
} );

it( 'should return empty state if last block inserted is called with action RESET_BLOCKS', () => {
const expectedState = {};

const action = {
type: 'RESET_BLOCKS',
};

const state = lastBlockInserted( expectedState, action );

expect( state ).toEqual( expectedState );
} );
} );
} );
51 changes: 51 additions & 0 deletions packages/block-editor/src/store/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ const {
__experimentalGetPatternsByBlockTypes,
__unstableGetClientIdWithClientIdsTree,
__unstableGetClientIdsTree,
wasBlockJustInserted,
} = selectors;

describe( 'selectors', () => {
Expand Down Expand Up @@ -3481,6 +3482,56 @@ describe( 'selectors', () => {
);
} );
} );

describe( 'wasBlockJustInserted', () => {
it( 'should return true if the client id passed to wasBlockJustInserted is found within the state', () => {
const expectedClientId = '62bfef6e-d5e9-43ba-b7f9-c77cf354141f';
const source = 'inserter_menu';

const state = {
lastBlockInserted: {
clientId: expectedClientId,
source,
},
};

expect(
wasBlockJustInserted( state, expectedClientId, source )
).toBe( true );
} );

it( 'should return false if the client id passed to wasBlockJustInserted is not found within the state', () => {
const expectedClientId = '62bfef6e-d5e9-43ba-b7f9-c77cf354141f';
const unexpectedClientId = '62bfsed4-d5e9-43ba-b7f9-c77cf565756s';
const source = 'inserter_menu';

const state = {
lastBlockInserted: {
clientId: unexpectedClientId,
source,
},
};

expect(
wasBlockJustInserted( state, expectedClientId, source )
).toBe( false );
} );

it( 'should return false if the source passed to wasBlockJustInserted is not found within the state', () => {
const clientId = '62bfef6e-d5e9-43ba-b7f9-c77cf354141f';
const expectedSource = 'inserter_menu';

const state = {
lastBlockInserted: {
clientId,
},
};

expect(
wasBlockJustInserted( state, clientId, expectedSource )
).toBe( false );
} );
} );
} );

describe( '__experimentalGetParsedReusableBlock', () => {
Expand Down
Loading