Try/deleting multi blocks #3194
Conversation
|
One thing I was unsure of how to do was focus the post title if there are no blocks. |
Codecov Report
@@ Coverage Diff @@
## master #3194 +/- ##
==========================================
+ Coverage 31.64% 31.81% +0.16%
==========================================
Files 217 217
Lines 6282 6296 +14
Branches 1116 1119 +3
==========================================
+ Hits 1988 2003 +15
+ Misses 3610 3609 -1
Partials 684 684
Continue to review full report at Codecov.
|
| @@ -285,6 +285,22 @@ export function removeBlocks( uids ) { | |||
| } | |||
|
|
|||
| /** | |||
| * Returns an action object used in signalling that the blocks | |||
| * corresponding to the specified UID set are to be removed, and then | |||
| * another block given focus. It differs from removeBlocks in that it | |||
There was a problem hiding this comment.
Do we really want a new action. Could we just use a flag?
There was a problem hiding this comment.
Yes, we shouldn't have two concurrent actions with hard-to-distinguish semantics.
In the new DELETE_BLOCKS effect, the removal itself is not conditional on state (only on payload, with the check for blockUids.length). Therefore, we should keep a single action, REMOVE_BLOCKS, and add a small effect for it that handles the new focus.
Now, the PR deals with two separate things:
- Fixing deletion in multi-block selection
- Focusing siblings after deletion
The former is a bug fix, the latter a UX improvement. I like it, but it begs the question: if we want to refocus a block after a multi-block deletion, are there any other cases (single-block deletion) where we don't want to refocus a block? Should we prefer a consistent behavior, whether that means always refocusing or never refocusing? cc @jasmussen
If still we want to make a distinction, then our effects.REMOVE_BLOCKS could look like:
if ( blockUids.length > 1 ) {
const blockToFocus = …;
dispatch( focusBlock( blockToFocus.uid ) );
}There was a problem hiding this comment.
Do we really want a new action. Could we just use a flag?
I've generally found flags are much harder to reason about later, and don't scale at all --- but that's my experience. I'm happy to go with the majority.
Or dispatch two actions?
I incorrectly assumed that for multi-actions, that effects is where they should go. I'm now realising that effects is for more complicated things. I don't think I'd seen many examples of where more than one action was dispatched in a component, so I thought that if you needed to do that, you put it in effects. My mistake.
are there any other cases (single-block deletion) where we don't want to refocus a block?
My rationale for keeping the old behaviour and adding new behaviour is that you never want to enforce focus in an action. You might find that something starts removing blocks asynchronously, and it can not focus grab from where you are. We could do some other logic, like only focus another block if we know for sure the focus was in one of the blocks to be removed (because nothing about the remove semantics enforces that they were selected), but I would strongly advise against always refocusing a block on remove unless it's a bit smarter than just "always".
In regards to the actions, effects separation, I'd really welcome something fixing it with a commit, and then I can look at the diff and see by example what your preferred approach is.
|
So after reading @iseulde 's comment (#3191 (comment)), I'm wondering if this is the approach we want to take. What this will do is focus the selectionStart outer container (but not the editable inside it). Therefore, the keyboard handling is picked up at the outer container level. It's quite likely that at some point, we chose to focus the block settings button instead of the outer container. This was probably done by setting focus to be This does highlight an advantage of storing all the known focus information in the redux state like #3195 was exploring, though. You can choose explicitly what to focus after firing a multi-select by just setting the focus appropriately in the action (e.g. |
|
I would prefer if we go back to focussing an action in a series of actions for multi-selection, as that's the next thing you would want to do, even if it's just clearing or extending the selection. Setting focus on one of the selected blocks is meaningless in this "mode". I'll open a new PR with an alternative fix that restores the previous behaviour. |
|
Okay, I created #3222. |
|
@iseulde there are advantages and disadvantages I can see to focusing an action after a multi-select. Let me know if you agree? Advantages
Disadvantages
So this disadvantage that I see is that selecting a button to me conveys that we are no longer able to modify our selection or interact with it outside activating button options. That's just my opinion, though. |
|
This ticket is a big confusing to me, so please bear with me as I ask silly questions. Is the primary purpose of this ticket to ensure that the Delete key deletes multi-selected blocks? If so, yay, because that's broken now.
Just to try and understand, please correct me if I'm wrong. This concerns the following behavior?
Is the question then: "where do we put focus now"? If that is the question, I think the answer has to tie into the navigation mode/editing mode. As soon as you multi select, you are in navigation mode, and you can use the arrow keys to set focus on a block. This is useful for when you want to use keyboards to manually select or unselect individual blocks. The answer to what happens when you delete is that you fall back to this navigation mode arrow key focus being on the block before the blocks deleted, or the first block otherwise. Does that make sense? |
That may be true, but I don't really thing it's a problem? I guess ideally we'd focus the selection of blocks, but certainly not just one? cc @afercia Maybe you have insights. :) |
|
@ephox-mogran Do you think we still need pieces from this PR? |
|
Not sure @iseulde |
|
Closing as superseded by #3253 |
Description
Fix for #3191
Stopped focusing inside editors when multi-select was enabled, and focused an appropriate block when the current block was deleted.
How Has This Been Tested?
Wrote a test for the
effects.jsadditionDELETE_BLOCKSScreenshots (jpeg or gifs if applicable):
Types of changes
Introduced another effect
Stopped passing focus through to blockType.edit when we have a multi-selection
Focused a block after deleting
Checklist: