Block List: Hide hover effects when typing #5106
Conversation
Multi-selecting and singular selection shouldn't have hover effects applied
aduth
added
[Type] Bug
| const { isHovered } = this.state; | ||
|
|
||
| if ( isHovered || isMultiSelected || this.hadTouchStart ) { | ||
| if ( isHovered || isMultiSelected || isSelected || |
There was a problem hiding this comment.
I explicitly removed isSelected here on a previous PR. Let me try to find the reason for this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it's not problematic anymore since #5050
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
isTypingchecks 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.
| @@ -145,6 +145,10 @@ export class BlockListBlock extends Component { | |||
| ) { | |||
| this.previousOffset = this.node.getBoundingClientRect().top; | |||
| } | |||
|
|
|||
| if ( newProps.isTyping ) { | |||
There was a problem hiding this comment.
Don't we need to compare with the previous prop or is unnecessary?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
hideHoverEffects is itself a no-op if the state is already set
There was a problem hiding this comment.
Ah, GitHub didn't update fast enough to show Andrew's reply. :)
| const { isHovered } = this.state; | ||
|
|
||
| if ( isHovered || isMultiSelected || this.hadTouchStart ) { | ||
| if ( isHovered || isMultiSelected || isSelected || | ||
| this.props.isMultiSelecting || this.hadTouchStart ) { |
There was a problem hiding this comment.
Minor, but the other props are extracted via destructuring assignment, while isMultiSelecting isn't.
There was a problem hiding this comment.
Minor, but the other props are extracted via destructuring assignment, while
isMultiSelectingisn't.
Our good friend no-shadow doesn't like the destructuring 😄
Regression introduced in #5078
This pull request seeks to resolve an issue where typing in a block does not dismiss hover effects. This behavior existed prior to #5078. Further, it avoids applying hover effects in more cases: when multi-selecting, or the block is selected (since block chrome is shown anyways).
Testing instructions: