Made gallery images unselect when clicking outside the block or selecting other image. #4903
Conversation
blocks/library/gallery/block.js
Outdated
| if ( event.target.tagName === 'FIGCAPTION' ) { | ||
| return () => { | ||
| // if the image is already selected do nothing | ||
| if ( this.state.selectedImage === index ) { |
There was a problem hiding this comment.
With this added, the toolbar will remain at the caption field when clicking the related image. Never reaches the setFocus call. Here's what I suggested: #4199 (comment).
There was a problem hiding this comment.
Ah ok, thank you @iseulde I got the idea of the behavior you are suggesting and I totally agree with it.
I updated the code to follow that behavior although in this behavior we still have to check if the clicks are in FIGCAPTION element because otherwise each time we do actions in the RichText we will lose the focus of it.
jorgefilipecosta
force-pushed
the
update/changed-select-bahaviour-gallery
branch
from
52b4a3b to
61fba31
Compare
|
Hm, you're right. Looked at the image block a bit. I see now that there is |
jorgefilipecosta
force-pushed
the
update/changed-select-bahaviour-gallery
branch
2 times, most recently
from
6e0789a to
36e042c
Compare
|
This PR had to be rebased. Besides changing the selection behavior it also solves a bug regression introduced since the merge of the captions. @iseulde thank you for your suggestion following a similar idea to the one you provided the onClick was changed to the image like in the image block, so the code to detect clicks on figcaption can safely be removed. |
blocks/library/gallery/block.js
Outdated
| this.setState( ( state ) => { | ||
| // no state change required | ||
| if ( state.selectedImage === index && ! state.captionSelected ) { | ||
| return; |
There was a problem hiding this comment.
Hm, isn't this equivalent to this.setState( undefined )? Shouldn't we avoid these checks inside the setState call? Is there much benefit in adding the check at all?
There was a problem hiding this comment.
Thank you for the feedback @iseulde, In the new approach, checkes are made outside setState, so we are certain no state change occurs if not needed.
blocks/library/gallery/block.js
Outdated
| @@ -51,33 +52,38 @@ class GalleryBlock extends Component { | |||
|
|
|||
| this.state = { | |||
There was a problem hiding this comment.
Do you think we could move this entire state to GalleryImage? It doesn't really seem needed this far up, and we'd avoid passing so much down the component tree.
There was a problem hiding this comment.
I think it would also avoid onSelectCaption and onSelectCaption being wrapped functions.
There was a problem hiding this comment.
Hi @iseulde, I agree that it makes sense to move the captionSelected state to the GalleryImage and I updated the code. Regarding the index of the currently selected image I kept it in Gallery block because when we select an image we automatically unselect another image, that index affects the state of multiple images so it makes sense to stay in the parent.
Simplified code because the onClick is on the image so there is no need to detect clicks on figcaption. Corrected bug where the caption field automatically selects when selecting image. Implemented a caption select state in GalleryImage.
jorgefilipecosta
force-pushed
the
update/changed-select-bahaviour-gallery
branch
from
36e042c to
eb3fd81
Compare
Currently, gallery images, are unselected when clicking on images already selected. This change makes the block more consistent with other blocks.
This PR comes from a suggestion of @iseulde in PR #4199. A different PR was created so #4199 does not introduce behavior changes to the gallery just adds the captions.
How Has This Been Tested?
Add a gallery with images.
Select an image click again on the image, verify it does not unselect while clicking.
Click on other image verify the selected image changes.
Click outside the block verify images get unselected.
Focus a caption verify the edition toolbar appears, click on the image outside the caption verify the image is still selected but the toolbar disappears.