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: Hide hover effects when typing #5106

Merged
merged 3 commits into from
Feb 16, 2018
Merged

Block List: Hide hover effects when typing #5106

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1d6e35
Block List: Hide hover effects when typing
aduth Feb 16, 2018
5ae6d97
Block List: Avoid hover state on all selections
aduth Feb 16, 2018
d4b93b4
Block List: Unset hover state when block is selected
aduth Feb 16, 2018
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
15 changes: 10 additions & 5 deletions editor/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class BlockListBlock extends Component {
this.bindBlockNode = this.bindBlockNode.bind( this );
this.setAttributes = this.setAttributes.bind( this );
this.maybeHover = this.maybeHover.bind( this );
this.onMouseLeave = this.onMouseLeave.bind( this );
this.hideHoverEffects = this.hideHoverEffects.bind( this );
this.maybeStartTyping = this.maybeStartTyping.bind( this );
this.stopTypingOnMouseMove = this.stopTypingOnMouseMove.bind( this );
this.mergeBlocks = this.mergeBlocks.bind( this );
Expand Down Expand Up @@ -145,6 +145,10 @@ export class BlockListBlock extends Component {
) {
this.previousOffset = this.node.getBoundingClientRect().top;
}

if ( newProps.isTyping ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to compare with the previous prop or is unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but it would have no impact, since hideHoverEffects guards against unnecessary setState, so even though this would repeatedly call hideHoverEffects, it would immediately abort because it's not hovered anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

hideHoverEffects is itself a no-op if the state is already set

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, GitHub didn't update fast enough to show Andrew's reply. :)

this.hideHoverEffects();
}
}

componentDidUpdate( prevProps ) {
Expand Down Expand Up @@ -286,10 +290,11 @@ export class BlockListBlock extends Component {
* @see https://developer.mozilla.org/en-US/docs/Web/Events/mouseenter
*/
maybeHover() {
const { isMultiSelected } = this.props;
const { isMultiSelected, isSelected } = this.props;
const { isHovered } = this.state;

if ( isHovered || isMultiSelected || this.hadTouchStart ) {
if ( isHovered || isMultiSelected || isSelected ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I explicitly removed isSelected here on a previous PR. Let me try to find the reason for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, to mimic the reducer behavior removed in #5078, we could hide the hover effects when the selected block changes the same way we're doing isTyping checks in this branch.

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 not problematic anymore since #5050

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 didn't really notice any bugs here aside from hitting a debugger breakpoint when I wasn't expecting it. The multi-selection one had some small issue previously where ending a multi-selection would cause hover effects on the starting and ending blocks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, to mimic the reducer behavior removed in #5078, we could hide the hover effects when the selected block changes the same way we're doing isTyping checks in this branch.

We might want this anyways. Noting another small issue is that when splitting a block while currently hovered over the original, hover effects appear above the original.

this.props.isMultiSelecting || this.hadTouchStart ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but the other props are extracted via destructuring assignment, while isMultiSelecting isn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Minor, but the other props are extracted via destructuring assignment, while isMultiSelecting isn't.

Our good friend no-shadow doesn't like the destructuring 😄

return;
}

Expand All @@ -301,7 +306,7 @@ export class BlockListBlock extends Component {
* where mouseleave may occur but the block is not hovered (multi-select),
* so to avoid unnecesary renders, the state is only set if hovered.
*/
onMouseLeave() {
hideHoverEffects() {
if ( this.state.isHovered ) {
this.setState( { isHovered: false } );
}
Expand Down Expand Up @@ -565,7 +570,7 @@ export class BlockListBlock extends Component {
<IgnoreNestedEvents
ref={ this.setBlockListRef }
onMouseOver={ this.maybeHover }
onMouseLeave={ this.onMouseLeave }
onMouseLeave={ this.hideHoverEffects }
className={ wrapperClassName }
data-type={ block.name }
onTouchStart={ this.onTouchStart }
Expand Down