-
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
Extend updateBlockAttributes to provide for different attribute changes for each block in the clientIds array #29099
Extend updateBlockAttributes to provide for different attribute changes for each block in the clientIds array #29099
Conversation
…es for each block in the clientIds array
Size Change: +2.44 kB (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
@@ -822,7 +822,9 @@ export const blocks = flow( | |||
( accumulator, id ) => ( { | |||
...accumulator, | |||
[ id ]: reduce( | |||
action.attributes, | |||
action.attributes[ id ] |
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 seems a bit weird to rely on the "id" for this check. What if there's an attribute that conflicts with the "id" (both are strings). I wonder if it's better to be explicit here and have a separate action (or a flag).
cc @ellatrix
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 I guess I was thinking that there was no chance of an attribute name being in the format of "b4c1d31e-fba3-4438-8d87-8d3554d8c4fb"
and randomly matching an actual block clientId.
I think you are right though, the API is better if we are more explicit. It seems like the two options are too close to warrant a whole separate action so have added a uniqueByBlock
flag. Let me know if you think this is a suitable way forward and I will extend the unit tests.
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, maybe we could add a unit test for that.
Description
In the gallery refactor PR we are looking for a way to
undo
all changes to child image attributes in one go. UsingupdateBlockAttributes
allows this, but currently all the blocks passed to it must have the same attributes. The gallery changes required that each child block update has slightly different attributes, eg. the unique media file url.The PR extends
updateBlockAttributes
to allow the attributes param to alternatively have attributes keyed by clientId.How has this been tested?
Currently just manually tested against some changes in gallery PR. Will add unit tests if this approach is approved.