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.

Sign up for GitHub

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

Jump to bottom

Block Editor: move selection state from RichText to the store #14640

Merged
merged 3 commits into from
Apr 19, 2019
Merged

Block Editor: move selection state from RichText to the store #14640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be42d1f
Block Editor: move selection state from RichText to the store
ellatrix Apr 4, 2019
e2d9c9c
Add docs
ellatrix Apr 17, 2019
0008d0e
Rename
ellatrix Apr 19, 2019
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
38 changes: 38 additions & 0 deletions docs/designers-developers/developers/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,32 @@ Returns the number of blocks currently present in the post.

Number of blocks in the post.

### getSelectionStart

Returns the current selection start block client ID, attribute key and text
offset.

*Parameters*

* state: Block editor state.

*Returns*

Selection start information.
ellatrix marked this conversation as resolved.
Show resolved Hide resolved

### getSelectionEnd

Returns the current selection end block client ID, attribute key and text
offset.

*Parameters*

* state: Block editor state.

*Returns*

Selection end information.

### getBlockSelectionStart

Returns the current block selection start. This value may be null, and it
Expand Down Expand Up @@ -1042,6 +1068,18 @@ Returns an action object used in signalling that the caret has entered formatted

Returns an action object used in signalling that the user caret has exited formatted text.

### selectionChange
Copy link
Member

Choose a reason for hiding this comment

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

To maximize consistency with other action names in the pattern of "verbNoun" / "actSubject" (insertBlock, updateSettings, clearSelectedBlock), in retrospect I think this should have been named as changeSelection. Has this landed in a stable WordPress release?


Returns an action object used in signalling that the user caret has changed
position.

*Parameters*

* clientId: The selected block client ID.
* attributeKey: The selected block attribute key.
* startOffset: The start offset.
* endOffset: The end offset.

### insertDefaultBlock

Returns an action object used in signalling that a new block of the default
Expand Down
2 changes: 0 additions & 2 deletions packages/block-editor/src/components/block-controls/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@ exports[`BlockControls should render a dynamic toolbar of controls 1`] = `
value={
Object {
"clientId": undefined,
"focusedElement": null,
"isSelected": true,
"name": undefined,
"onFocus": undefined,
"setFocusedElement": [Function],
}
}
>
Expand Down
49 changes: 21 additions & 28 deletions packages/block-editor/src/components/block-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import memize from 'memize';

/**
* WordPress dependencies
*/
Expand All @@ -10,40 +15,28 @@ import Edit from './edit';
import { BlockEditContextProvider } from './context';

class BlockEdit extends Component {
constructor( props ) {
super( props );

this.setFocusedElement = this.setFocusedElement.bind( this );

this.state = {
focusedElement: null,
setFocusedElement: this.setFocusedElement,
};
constructor() {
super( ...arguments );

// It is important to return the same object if props haven't changed
// to avoid unnecessary rerenders.
// See https://reactjs.org/docs/context.html#caveats.
this.propsToContext = memize(
this.propsToContext.bind( this ),
{ maxSize: 1 }
);
}

setFocusedElement( focusedElement ) {
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
this.setState( ( prevState ) => {
if ( prevState.focusedElement === focusedElement ) {
return null;
}
return { focusedElement };
} );
}

static getDerivedStateFromProps( props ) {
const { clientId, name, isSelected, onFocus } = props;

return {
name,
isSelected,
clientId,
onFocus,
};
propsToContext( name, isSelected, clientId, onFocus ) {
return { name, isSelected, clientId, onFocus };
}

render() {
const { name, isSelected, clientId, onFocus } = this.props;
const value = this.propsToContext( name, isSelected, clientId, onFocus );

return (
<BlockEditContextProvider value={ this.state }>
<BlockEditContextProvider value={ value }>
<Edit { ...this.props } />
</BlockEditContextProvider>
);
Expand Down
9 changes: 1 addition & 8 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
focus,
isTextField,
placeCaretAtHorizontalEdge,
placeCaretAtVerticalEdge,
} from '@wordpress/dom';
import { BACKSPACE, DELETE, ENTER } from '@wordpress/keycodes';
import {
Expand Down Expand Up @@ -154,13 +153,7 @@ export class BlockListBlock extends Component {
return;
}

target.focus();

// In reverse case, need to explicitly place caret position.
if ( isReverse ) {
placeCaretAtHorizontalEdge( target, true );
placeCaretAtVerticalEdge( target, true );
}
placeCaretAtHorizontalEdge( target, isReverse );
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, how it has changed in a way that it no longer needs to use placeCaretAtVerticalEdge?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was a mistake before. It doesn't make sense to call both at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this was a mistake as we iterated on this on a few occasions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why placeCaretAtVerticalEdge is used here. It's meant to place the caret at the start or end of a (rich) text area.

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 the idea here is to place the caret at the end of the block's last textarea/rich-text

Not against the refactoring but the intent is to move to the end of the block (which means the end of RichText if the block is a RichText based block)

Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad Yes, I know. placeCaretAtHorizontalEdge accomplishes exactly that on its own. placeCaretAtVerticalEdge seems unnecessary here.

}

setAttributes( attributes ) {
Expand Down
Loading