-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Gallery: Convert Gallery block to use Image blocks instead of having its own nested image format #25940
Merged
glendaviesnz
merged 158 commits into
trunk
from
refactor/gallery-to-nested-image-blocks
Aug 19, 2021
Merged
Gallery: Convert Gallery block to use Image blocks instead of having its own nested image format #25940
Changes from 1 commit
Commits
Show all changes
158 commits
Select commit
Hold shift + click to select a range
c01bbfb
Refactor gallery to nested image blocks
038b22d
Fix issue with images not loading on first selection from media gallery
331c302
Remove the default columns setting as we don't have access to innerBl…
ee7da0a
Revert "Remove the default columns setting as we don't have access to…
c9fe911
Add image count so we can work out default columns as innerBlocks not…
a00a544
Disable the innerBlocks dropzones so drag drop works same as existing…
0c8742f
Lint changes
a77854c
Revert "Lint changes"
1965514
Revert "Disable the innerBlocks dropzones so drag drop works same as …
e5e1420
Suggested solution for handling multiple file drop into gallery
1d6c559
Remove non image files from drag and drop and disable individual imag…
39cecec
Fix transform to individual images
d9ec2b0
Fix transform from individual images
3742af8
Revert drag and drop transform changes
3acb223
Add gallery transform to image block to override the default gallery …
084e6c7
Move handling of file uploads to Gallery from media placeholder
cff6727
split innerblocks mapping into separate effect to reduce chatter
f1606cc
Add useMemo to currentImageOptions
1140646
reuse existing innerBlocks rather than recreating with every new imag…
6d23628
Switch to useMemo for updating local image const instead of local com…
f7a1ff4
Fix issue with image sizing not being available on initial load of co…
974ad90
Memoise the useImageSizes hook
5ac143b
Fix issue with media browser defaulting to edit gallery view
8ceaa3d
Fix missed incorrect use of addToGallery
8d7159b
Add some extra effects for getting the imageData as the getMedia call…
dbda7e9
Simplify the imageData by using a useSelect
342695e
Another optimisation - only return a new imageData reference if all i…
0744b59
Refactored Gallery: Add loading state to gallery image size options (…
aaronrobertshaw dad1b0b
Initial deprecations commit
bcf6664
Fix issue with linkDestination not being applied in migration
921fecc
Refactor gallery deprecations
aaronrobertshaw 7920a45
Fix missing attributes from migration
aaronrobertshaw c97630c
Update deprecation to set allowResize
aaronrobertshaw 2724965
Fix issue with crop not working when certain plugins are loaded
098ba86
Fix SCSS lint errors
aaronrobertshaw 49f9d6b
Update the block example
88c096b
Linting fixes
37b4b4e
Fix the e2e test and the accessibility issue with having aria group r…
0c5e720
Fix the e2e test and the accessibility issue with having aria group r…
5840a1c
Fix frontend omission of wp-block-image class
aaronrobertshaw e1e6a8a
change deprecation to use imageCount as isEligible check as it seems …
d7187b4
Move back to a single deprecations file and reorder in array
b648b10
Remove additional check in v5 isEligible
f1454f2
Fix the v4 migration
e76bb06
Fix styles for Safari compatibility
aaronrobertshaw b969e3f
Remove unnecessary gallery editor styles
aaronrobertshaw 0ff156a
Fix typo in deprecations
aaronrobertshaw 309f9f0
Restore styles to render deprecated gallery versions
e6d1301
Avoid applying flex styles to IE11
aaronrobertshaw 4953a0c
Add additional selector to prevent the hidden individual image drop z…
d259127
IE11 styling improvements
aaronrobertshaw 1a16354
Apply default style class to new images added to gallery
9aadf8f
fix linting issues
e879978
Move block props to the outer wrapper
f072943
Revert "Move block props to the outer wrapper"
c4d06ee
Revert "Revert "Move block props to the outer wrapper""
82eee6e
Fetch media if isListItem is true
601119c
Change context from isListItem to isGrouped
4f1ddb4
Remove wrapper div
a6603cf
remove extra wrapper around media placeholder and caption and use fle…
9b8d498
Revert "remove extra wrapper around media placeholder and caption and…
bd88cb2
Revert "Remove external div wrapper by moving InnerBlocks to a fragment"
abcb029
another update to image wrapper
939456d
put media uploader outside figure so structure matches front end
bf0aec2
Replace div with View for the sake of native code
0938b5e
Move setting of attributes to the child images
94d9877
Gallery Block Refactor: Account for null image ids in gallery migrati…
glendaviesnz 17ca088
Remove the gradient and put caption under image if is-rounded style a…
glendaviesnz 2c2b38e
Remove outer div wrapper
82ef52a
Keep image margins while dragging sibling
aaronrobertshaw bc61288
Fix e2e test expected markup to match new structure
7af15db
Gallery Block Refactor: Add handling of short code transforms
1e1752e
Removed unused prop
726e48c
Account for undefined block and innerblocks in conversion to reusable…
21bd032
Add custom gutter sizes to refactored gallery (#28377)
aaronrobertshaw a9453d2
Adjust editor styles to match new dom structure
83b9074
Remove redundant styles that are duplicated in nested image blocks
e3a6c29
Fix issue with Image block dragged out of Gallery still having inheri…
d34dcdb
When dragging an image block into a gallery make sure we don't wipe a…
72e9c2b
fix issue with variable declaration order
e291f91
Fix bug with custom link being overwritten by gallery linkTo changes
795a568
Fix application of gutter size CSS var (#28759)
aaronrobertshaw 9580649
Fix mobile width for gallery images
aaronrobertshaw 74072f0
Add missing dependency to inner images selector
aaronrobertshaw cf4dc34
remove conversion to cover block if image in gallery
f90965c
Add fallback to old gallery edit and save for existing gallery (#28961)
glendaviesnz be75a36
Remove duplicate import
780e9e7
Remove need for temporary imageUploads attribute as we can just creat…
fd1e27d
Remove handling of gallery attribute updates from child images
652557e
Move updating of attributes back to gallery and show snackbar to indi…
1da955d
Update transforms to work with both versions of gallery
5b809d5
Remove redundant changes
b3fb72c
Remove redundant changes
b5ca5ef
Remove unused method
d5f5cba
Merge similar Image transforms.
82b97b8
Fix some issues missed when moving attribute setting back to gallery.
d9b1733
Simplify handling of images from media placeholder now that drag and …
ee6bef7
Fix typo
aaronrobertshaw 9e0b05c
Fix broken upload from media placeholder
f27e872
Fix issue with new file uploads overwriting existing blocks
30dc02f
Remove isGrouped dependency from call to get image as not needed now …
3d47210
Remove custom gutter size feature
aaronrobertshaw fd56523
Use getMediaItems instead of getMedia for getting image data for all …
657ba78
Add back missing context prop from rebase conflict
a111532
Make use of new uniqueByBlock flag on updateBlockAttributes to batch …
6370194
Move the setting of the default attributes on new blocks to a single …
glendaviesnz e23ddb0
Add snackbar notice ids (#29364)
aaronrobertshaw fc2b2a0
Account for people adding and removing images from media browser so …
716ea1b
Changes from PR feedback
44df5dd
Fix issue with deprecated constants
3a75fb0
Fix linting and e2e test failures
84ebe2e
Some more e2e fixes
6b06732
Add transform from old gallery to new format
6caef45
Memoize the allowedBlocks
aaronrobertshaw 8eceac7
Add warning about image formats required if uploading to gallery
efd0705
Move allowedBlocks outside of component to avoid useMemo use
aaronrobertshaw 82cf526
Re-apply uncropped alignment changes lost in rebase
d133db7
Fix issue with non-cropped images in deprecated gallery
651ff59
Don't show media library buttons while images are still uploading as …
6209e6e
Remove gallerRef that was no longer doing anything
eaa0c7a
Respect sort order from Media Library (#30070)
glendaviesnz 20477de
Gallery block refactor: make invalid file type errors consistent (#30…
glendaviesnz 6195768
Fix issue with invalid type message when adding files via media brows…
9cfb170
Gallery block refactor: check for new images by clientId instead of i…
glendaviesnz 772691d
Apply changes from rebase to deprecated gallery
d71011b
Apply patch from rebase to not select all items in media library when…
991437d
Copy caption across from image selected from media library (#30784)
glendaviesnz a2253ec
Add data-id to image to help with compatibility of refactored gallery…
glendaviesnz d225aca
Revert "Add data-id to image to help with compatibility of refactored…
fc06639
fix bug with image style being lost when gallery grouped (#31068)
glendaviesnz 1067a0c
Only add RichText component if the figcaption is clicked to prevent i…
glendaviesnz ff4fadc
Fix bug with alt text not being copied from media library (#31066)
glendaviesnz b976235
Merge branch 'trunk' of github.com:WordPress/gutenberg into refactor/…
1a4345a
Gallery refactor - Infer version from existing content (#32270)
mkevins e5420c6
Merge branch 'trunk' into refactor/gallery-to-nested-image-blocks-ups…
mkevins e183dcb
Resolve merge conflicts
mkevins b2031f2
Use non-deprecated hook when possible
mkevins 71b249c
Merge branch 'trunk' into refactor/gallery-to-nested-image-blocks-ups…
mkevins c72984f
Resolve conflict in block editor default settings
mkevins 7da0c1b
Merge branch 'trunk' into refactor/gallery-to-nested-image-blocks-ups…
mkevins 797cb28
Resolve conflict in image.js
mkevins fad851a
Minor changes from code review:
a83b106
Improve naming of save and deprecated save methods
a424509
Rename __experimentalGalleryRefactor flag to __unstableGalleryWithInn…
c69f2d5
Remove the isGrouped context as no longer needed.
887b6b4
Rename __unstableGalleryWithInnerBlocks to _unstableGalleryWithImageB…
c4eb362
Gallery block refactor: remove the imageCount attribute (#33677)
glendaviesnz 55e8bf6
Merge branch 'trunk' of github.com:WordPress/gutenberg into refactor/…
8b1bd7d
Fix broken scss
0b9d8b2
Fix php linting error
6a58538
Merge mobile refactor of gallery to nested image blocks into desktop …
mkevins 8529284
Merge branch 'trunk' of github.com:WordPress/gutenberg into refactor/…
a41edeb
Fix issue with upload spinner not displaying correctly on v1 gallery …
0273b7b
Gallery block: Add a filter to automatically convert transforms to an…
glendaviesnz 5cbaedd
Merge branch 'trunk' of github.com:WordPress/gutenberg into refactor/…
78472d9
Fix issue with image to cover block conversion
fd0f7ec
Add imageCrop to gallery context
mkevins d55be9e
Fix exceptions caused by useBlocksProps running after block has been …
glendaviesnz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fetch media if isListItem is true
- Loading branch information
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
export const LINK_DESTINATION_NONE = 'none'; | ||
export const LINK_DESTINATION_MEDIA = 'file'; | ||
export const LINK_DESTINATION_ATTACHMENT = 'post'; | ||
export const LINK_DESTINATION_MEDIA = 'media'; | ||
export const LINK_DESTINATION_ATTACHMENT = 'attachment'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 guess these are meant to be passed down to the images to keep the same value for all images? Can't we start simpler instead and just let the images handle these. On the UI we can still target the children blocks and call their own
setAttributes
instead of setting the attribute on the Gallery block instead.I'd love if this PR to be as impact less as possible on the image block.
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.
We have been through quite a few iterations of this, and this seems to be the least impactful way on the image block while still providing the UX that uses expect based on our first round of testing, ie. users want to:
While point one is possible by setting the child attributes from the parent, point two is difficult as it is impossible to know from the parent which of the children might have had their settings manually toggled. We spent a bit of time working with @jasmussen on the UX around this ... but open to suggestions on alternatives.
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.
This is actually something I want to challenge. If you multiselect something (regardless of the context), say "text", and you remove "bold", it's going to make everything "bold" not just what was edited globally before. So it seems to me that overriding the single image choice should be the best way to go here.
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.
@jasmussen did you have any thoughts of this from our previous discussions about the new gallery settings. You may remember that we did have a prompt on the gallery settings to confirm that that user wanted to overwrite all child settings, but you saw that as an antipattern, so we moved to moved tp the child images only applying gallery settings if they hadn't been individually set.
I see it being a gotcha for users who may set custom urls on a large number of images, and then maybe toggle the linkTo select at the gallery level maybe thinking it will only affect new images but it in fact wipes all their custom settings.
Very happy to discuss the options and approaches further though.
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 definitely think any blanket overwriting of custom urls on inner image blocks would be a bad experience for our users. Not only would it be unexpected but they might not even notice it right away, increasing the frustration.
Perhaps the distinction here is that the user isn't actively multi-selecting images. Therefore they aren't making the choice to alter those inner images directly.
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.
It's a key discussion, with trade-offs on both sides. Thanks for the ping again, and thanks also for refreshing my memory:
Yes, moving the logic to the image itself seemed like the best alternative to a "confirm" prompt.
On the flipside, Riad makes a good point:
This is a "destructive" change in the same way as overriding link destinations on images in a gallery would be. The key difference is the bold toggle is very visual, whereas the gallery change is mostly invisible. You wouldn't necessarily know that this one image in the corner, which you linked to a rickroll video, had its URL changed.
But to Riad's point again — maybe you should have assumed this because it's the same as the "bold" behavior. And maybe it is correct behavior after all. And maybe linking just one image is an edgecase not worth adding undue complexity for. And maybe we should embrace that there are highly visible undo/redo buttons in the toolbar.
Because I argued for the opposite, I refreshed the branch and took it for a spin to get a feel for it again:
So good. The fact that I can click an image to select it, press Delete to remove it directly. That I can rearrange these? This is all so good. And the "select parent" change really benefits this.
I tend to still think it would be nice to inform the user, somehow, that these gallery-level link changes might override that carefully constructed URL. But given we have undo, and version history, I do see where Riad is coming from.
Could a compromise be to go with Riad's approach, but to throw up a snackbar notice when you make a URL change? "All images in gallery now link to Media File"?
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.
@jasmussen, @youknowriad I have refactored in line with your suggestion Joen and the update now is handled by the gallery with a snackbar to indicate that all images were affected. The only issue is that each image update is a separate entry in the undo stack, so to undo the change in gallery with 3 images you need to hit undo 3 times. Do either of you know if there is a way to batch a series of changes so they can be undone in one go? I ran out of time to explore the code for any examples of this today.
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 we do support multiselection of blocks and changing attributes for all these blocks at the same time, so it seems both these two things should work the same way. I'm not sure whether this use-case creates a single undo or not though. If not, it should be as easy as having a new action on the block-editor package to update all the blocks in one go.
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.
Yeh, it turns out you can update multiple blocks with
updateBlockAttributes
by passing an array of clientIds, and it gets added as a single event in undo stack ... but all the attributes have to be the same, and unfortunately with the likes of linkTo changes the href attrib is different for every nested image block soupdateBlockAttributes
has to be run separately for each block.I will take a look at the option of adding a new action to the block-editor package tomorrow ... I assume that would be better as a standalone PR? New area for me so not sure how far I will get with that.
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.
@youknowriad it looks like this could be as easy as extending
updateBlockAttributes
to allow a clientId keyed object of attributes as an alternative to a flat object of attributes. I have a prototype of this at #29099, and this works when tested with gallery PR to allow all gallery level attribute changes to be undone in one go. If you think this looks like a way forward I will do some more testing and add some additional unit tests to the PR.