-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Don't allow to start dragging blocks if a lock all exists #7225
Don't allow to start dragging blocks if a lock all exists #7225
Conversation
@@ -646,6 +648,7 @@ const applyWithSelect = withSelect( ( select, { uid, rootUID } ) => { | |||
initialPosition: getSelectedBlocksInitialCaretPosition(), | |||
isEmptyDefaultBlock: block && isUnmodifiedDefaultBlock( block ), | |||
isPreviousBlockADefaultEmptyBlock: previousBlock && isUnmodifiedDefaultBlock( previousBlock ), | |||
templateLock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be isDraggable
or isMovable
?
|
ececee1
to
94783cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a few documentation tweaks would be good here.
Also, "draggable" isn't technically a word. Maybe we could do draggingAllowed
, canDrag
, or isAllowedToDrag
or just anything else that wouldn't upset spellcheck 😉
Nevermind, I can see it's already the name of the component and I'm late to that party. Still, a few documentation tweaks requested 😅
components/draggable/README.md
Outdated
|
||
- Type: `boolean` | ||
- Required: No | ||
- Default: `true` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double-space in here; could be single.
components/draggable/README.md
Outdated
@@ -6,6 +6,14 @@ | |||
|
|||
The component accepts the following props: | |||
|
|||
### draggable | |||
|
|||
If the component is in a draggable state or not. If false, the component is rendered but it is not possible to drag it. Defaults to true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If the component is in a draggable state or not." is a bit confusing, I think the point of this is to set the "dragability" of the block, right? Would this be better as:
### draggable
Allow the component to be dragged in the editor; defaults to `true`. If `false`, the component is rendered but it is not possible to drag the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it's very confusing that Draggable
component can't be draggable in some circumstances. Ideally, it shouldn't be rendered at all when it is not draggable.
@@ -502,6 +503,7 @@ export class BlockListBlock extends Component { | |||
onDragEnd={ this.onDragEnd } | |||
isDragging={ dragging } | |||
elementId={ blockElementId } | |||
draggable={ isMovable } // only in locking all moving blocks is totally impossible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this comment mean? I don't quite understand it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason why we can't perform the check for isMovable
outside of the component?
{ ! isMultiSelected && (
<BlockDraggable
...
/>
) }
We already have a precedence here, what is blocking us from adding another check in addition to isMultiSelected
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is that we depend on the styles set by BlockDraggable. Without BlockDraggable being rendered, it becomes almost impossible to get the delete and ellipsis buttons to appear. When multi-selecting it is not a problem because they appear there all the time even if the mouse is not close to the blocks.
I will try to apply a refactor to the styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to apply a refactor to the styles.
That would be awesome, thanks for looking into it 👍
94783cb
to
f393392
Compare
Hi @gziolo, @tofumatt I applied your suggestions, and now this PR does not change any existing component. BlockDraggable is rendered when in fact we can move the block. In order to make not rendering BlockDraggable possible, a CSS rule was added that makes blocks hover and select area not dependent on BlockDraggable being present. |
f393392
to
c03559f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what these comments mean. If they could be clarified the code looks good; I just don't know about the comments.
|
||
.editor-block-list__block { | ||
@include break-small { | ||
// use a wider available space for hovering/selecting on top level blocks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should use capital "U" and have a period at the end of the sentence 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I'm not sure what this (or the comment below) mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @tofumatt thank you for your review. I updated the comments, let me know if you find the new version of the comments more helpful :)
c03559f
to
db5f764
Compare
db5f764
to
627beea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
The new comments totally explain it! There were some whitespace issues and I tweaked the comments. If they look good to you merge away 😄
&:before { | ||
bottom: 0; | ||
content: ''; | ||
left: -$parent-block-padding - $block-padding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate you making this change, and the intent behind it.
It introduced a small regression, though, as it makes a horizontal scrollbar appear when a block is full-wide.
I discovered this while rebasing #7365. I will implement a fix in that branch, perhaps you can help me review it so we can get it in faster? Thanks.
Description
Fixes: #7191
Part of a general polishing to get #6993.
If a template lock "all" exists, the block cannot be moved anywhere. In that case, we should not allow drag a block to start. In master we allow the block to dragged around, but it could not be dropped anywhere. This PR's avoids to drag&drop to start in this case.
How has this been tested?
I verified that if no locking exists the blocks can be dragged around.
If template locking = insert the block can be dragged around but not dropped anywhere. This is an existing bug, in this locking, it should be possible to move the block. This bug is going to be addressed in a different PR.
If template locking = all, it is not possible to start the drag& drop in a block.
Verify that if a template lock all exists hover and selects still work as expected.