Move select toolbar to header #955
Conversation
Whaaaa? Hopefully not while driving! :D Love it, works great! I pushed a color change to the bottom toolbar. I can't deselect by clicking outside blocks, though — only with the close button (though the close button deselect behavior is correct). Also, every once in a while when selecting across blocks there is some flickering happening. I can't quite pin down what makes it happens, feels like it happens when you start selecting inside text and across image blocks, but not sure. |
|
I'm getting a React JS error when opening post settings, an error I'm not seeing in master:
|
Yeah, this happens in master too. I've experienced it too, but usually only in the beginning. I think it might be because the editors are still loading or something, I'm not quite sure. Should be looked into separately. |
|
@jasmussen I'm getting a React error in master too. |
Ooops, there was a typo in the code. |
|
@jasmussen The error comes from ccf8728, not this branch. |
mtias
added
[Feature] Blocks
|
@jasmussen Does this look ok to go in? |
editor/header/index.js
Outdated
| import { getSelectedBlocks } from '../selectors'; | ||
|
|
||
| function Header( { selectedBlocks } ) { | ||
| if ( selectedBlocks.length ) { |
There was a problem hiding this comment.
Do we have two concepts of selected blocks? One for singular selection, and one for multi-selection? Can we consolidate these?
There was a problem hiding this comment.
Yes, we do, also two separate entries in state, mostly to keep the selection object for a single block.
There was a problem hiding this comment.
If my confusion could serve as evidence, it would be good if we could better distinguish or eliminate the differences between the separate selected state.
There was a problem hiding this comment.
Sure. It sound like that's something for a separate ticket/PR as this code just reads the state.
editor/header/index.js
Outdated
|
|
||
| function Header( { selectedBlocks } ) { | ||
| if ( selectedBlocks.length ) { | ||
| return <MultiSelectHeader selectedBlocks={ selectedBlocks } />; |
There was a problem hiding this comment.
Depending to which extreme we want to take component self-sufficiency, <MultiSelectHeader /> is equally capable of retrieving the currently selected blocks. We could even adopt a pattern where we always render <MultiSelectHeader />, but it returns null if there's not a multiple selection.
Not really suggesting any change here, just food for thought.
There was a problem hiding this comment.
You're right here, what you're suggesting sounds way better.
| handleClickOutside() { | ||
| this.props.clearSelectedBlock(); | ||
| handleClickOutside( { target } ) { | ||
| if ( target.nodeName !== 'BUTTON' ) { |
There was a problem hiding this comment.
Is this specifically targeting the <MultiSelectHeader /> button? Are we trying to resolve a race condition between when deleting blocks from header, and clicking outside? Is there any other way we could ensure the "Delete" logic is triggered first? setTimeout is an option here; not great, but perhaps still better than targeting other elements on the page.
There was a problem hiding this comment.
I'll have a closer look at this.
Visually fine, yes, I defer to Andrew on the code. |
ellatrix
force-pushed
the
add/multi-select-sticky
branch
from
23a2c1a to
2b0696b
Compare
|
Concerns addressed, so I'm going to merge. Let's iterate in further PRs. |
Did this quick while on the road. 🚗
See #951.