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 List: Select block only if not already selected #4586

Merged
merged 1 commit into from
Jan 19, 2018
Merged

Block List: Select block only if not already selected #4586

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 21 additions & 2 deletions editor/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,17 @@ export class BlockListBlock extends Component {
this.props.onInsertBlocks( blocks, this.props.order + 1 );
}

/**
* Marks the block as selected when focused and not already selected. This
* specifically handles the case where block does not set focus on its own
* (via `setFocus`), typically if there is no focusable input in the block.
*
* @param {FocusEvent} event A focus event
*
* @returns {void}
*/
onFocus( event ) {
if ( event.target === this.node ) {
if ( event.target === this.node && ! this.props.isSelected ) {
this.props.onSelect();
}
}
Expand All @@ -293,6 +302,13 @@ export class BlockListBlock extends Component {
event.preventDefault();
}

/**
* Begins tracking cursor multi-selection when clicking down within block.
*
* @param {MouseEvent} event A mousedown event.
*
* @returns {void}
*/
onPointerDown( event ) {
// Not the main button.
// https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
Expand All @@ -307,7 +323,10 @@ export class BlockListBlock extends Component {
}
} else {
this.props.onSelectionStart( this.props.uid );
this.props.onSelect();

if ( ! this.props.isSelected ) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: instead of doing the checks every time we try to use onSelect, is doing the check on the onSelect creation in mapStateToProps using ownProps an option?
This has the advantage of making sure that future onSelect additions the check is not forgotten.

Copy link
Member Author

Choose a reason for hiding this comment

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

is doing the check on the onSelect creation in mapStateToProps using ownProps an option?

Not here, since isSelected doesn't exist in ownProps, it is provided through mapStateToProps.

It is technically possible using mergeProps, the third argument of connect:

https://github.com/reactjs/react-redux/blob/master/docs/api.md#connectmapstatetoprops-mapdispatchtoprops-mergeprops-options

...but it's a bit more (error-prone) work to override the default here, and I think it's sensible enough that the component itself should check isSelected prior to calling onSelect.

this.props.onSelect();
}
}
}

Expand Down