-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fixes #11949: Error: The block "xxx" can have a maximum of 3 keywords. #11953
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.
@jameelmoses thanks for your contribution. The change that was introduced causes one of the unit tests to fail. You can run locally npm run test-unit
to find out more details. It has to be updated to reflect code changes.
The changes proposed in this PR look good and it seems like a good compromise. I would be happy to get another opinion from @aduth or @youknowriad.
@gziolo I ran unit tests, and 2 failed but they are seemingly unrelated to my changes.
|
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 udated version of the code removes the check for keywords completely which is different than the initial proposal. I don’t think this is expected API change.
This PR still have some merge conflicts so that migt why those unrelated tests fail.
I proposed the change at #11949 (comment) . I'd welcome your feedback if you have any reservations, though. |
I missed the discussion in the issue and I reviewed based on the previous agreement. I’m fine with your proposal applied in this PR as is. Let’s make sure thete are no merge conflicts and proceed with what we have at the moment 👍 This might help to close similar PRs 😃 |
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 did a cursory review of the codebase to ensure there were no other references to the three-keyword maximum.
I found a reference as part of the @wordpress/rich-text
package, but I believe this to be an entirely separate thing, though presumably inspired by the blocks implementation. We should probably bring this into alignment, though I don't think it ought to block the work here (cc @iseulde).
gutenberg/packages/rich-text/src/register-format-type.js
Lines 118 to 123 in a615ce6
if ( 'keywords' in settings && settings.keywords.length > 3 ) { | |
window.console.error( | |
'The format "' + settings.name + '" can have a maximum of 3 keywords.' | |
); | |
return; | |
} |
As @gziolo noted, there are merge conflicts to resolve, but otherwise the changes look good to me.
I opened #13848 to resolve merge conflicts and merge this PR into master. @jameelmoses thanks for your contribution 🎉 |
Description
Fixes: #11949
Instead of failing to register a block if more than 3 keywords are given, only return the first 3 by slicing the keywords array.
How has this been tested?
npm run lint
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: