-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Blocks: Add the missing alignments classNames to the Cover Image Block's markup #4060
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
Conversation
aduth
left a comment
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.
Seems an extreme change to fix a minor bug 😕 But unless we want to relent that a class name change is not an invalidating change (it could have breaking impact) or that fixing bugs might invalidate old blocks, seems necessary.
blocks/library/cover-image/index.js
Outdated
| }, | ||
|
|
||
| deprecated: [ { | ||
| attributes: attributesDefinition, |
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.
Do you think it'd be reasonable for omitting the attributes property from a deprecated definition to default to the current attribute set, since we're not changing anything about it? I guess this could be a concern for future maintainability if we did decide to later change 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.
The problem we this, is that an undefined value means no attributes right now. I guess we can still use {} as a replacement. The same could be said for supports. I don't feel strongly either way. Both have drawbacks:
- The necessity to repeat the value if no changes
- The necessity to understand that
{}is different than omitting the property for the second option.
|
@aduth I think the default value is "center". What if we avoid adding the className if the alignment is "center" and avoid the "deprecated" declaration entirely? |
|
Is the default center? For images it would be |
|
mmmm, It's not. I guess I assumed it was "center" because I had a "centered" cover image when testing. So this makes things easier IMO. We can just drop the "deprecated" declaration as is. This could produce "invalid" blocks but the chance for it to happen is small. |
|
Yes, I also share the same view. That it is a lot of code to add and process to fix a very small bug for a relatively simple block. We should watch closely how deprecation feature is used for blocks and take an action if this becomes too complicated to manage. On the other hand, maybe we should always copy and paste the version that gets updated but put everything in their own file to make sure it doesn't pollute the existing solution. The fix itself looks good :) |
10c2375 to
54e11d8
Compare
54e11d8 to
4fe7a03
Compare
|
Ok, since the default use-case is not affected, I've removed the deprecated block's definition. What do you think about moving forward as is? |
gziolo
left a comment
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.
Let's apply this as it is :)
closes #2650
Tiny PR adding the missing alignment classNames to the cover image block. It also declares a "deprecated" version to avoid invalidating old cover image blocks.
Testing instructions