-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
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.
As you've updated the save()
method, existing blockquotes will break if the old save method isn't moved to a deprecated
property. This takes the form:
deprecated: [{
attributes: {
// old attributes
},
save({ attributes }) {
// old save method
},
}],
Since you've only appended to the original attributes without changing any existing ones, it may be worth defining the original attributes as an object outside of the block registration. That way, you can use it straight in the deprecation, and use lodash's assign
to compose the new attributes onto this for the updated block registration definition.
@@ -29,29 +55,26 @@ registerBlockType('benenson/quote', { | |||
supports: { | |||
className: false, | |||
}, | |||
attributes: { | |||
align: { | |||
attributes: assign({}, oldAttributes, { |
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 missed this in my original review—apologies—but it looks like the attribute citation
has been replaced with subText
. Whilst I understand the reasoning behind why this has been done, because you're not removing it from the attribute list, it's still being declared as a valid attribute.
So, I'd recommend using Lodash's omit
method to remove the citation
attribute from the new block attribute definitions, in addition to what you have here.
Syntax is: omit(object, 'path')
or omit(object, ['path1', 'path2'])
, or see the docs, if you're unfamiliar.
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.
subText
is a new attribute, citation
still exists within new and old save method at least if am reading the code correctly.
Let me know if am wrong.
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.
No, you're right, sorry. I misread the changes because I only looked at them in GitHub and not in the source itself.
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.
LGTM
Proposed Changes