Fix bug with getFontSizeClass crashing if passed a number #32720
Conversation
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
|
Size Change: +154 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
ntsekouras
added
[Type] Bug
ntsekouras
reviewed
Jun 16, 2021
There was a problem hiding this comment.
Thanks @glendaviesnz for your work here!
Should we better convert and log to inform devs only if a number is passed, instead of adding a comment there? font-size hook declares the attribute as a string so even if we fallback gracefully, I don't think it should be there for long.
This was referenced Jun 16, 2021
aaronrobertshaw
approved these changes
Jun 16, 2021
There was a problem hiding this comment.
LGTM 🚢
I tested this along with #32722.
Prior to applying this fix, I could replicate the errors, afterwards there were none.
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
👋 Changes look good. I echo Nik's comment on logging to console so devs know that a number isn't the expected input for this. |
|
@glendaviesnz @aaronrobertshaw I'd like to prepare a 10.8.2 that includes this and #32722 as well. I know it's late for you, so I was thinking about taking over these two PRs and get them ready to the patch release. |
|
Should we add some unit tests to cover #32719 (and maybe some other scenarios)? |
oandregal
force-pushed
the
fix/get-font-size-class-bug
branch
from
b9bbb59 to
208e089
Compare
I'll add some... |
oandregal
reviewed
Jun 16, 2021
|
I'm trying to add a unit test that checks if So I've tried the following: But apparently, it's now really throwing a console warning; instead, I'm getting the following test error: Any idea how to fix that? 🤔 |
I'll see myself out. |
ockham
requested changes
Jun 16, 2021
There was a problem hiding this comment.
The console warnings that we're getting (through deprecated()) are
"The font size slug should be a string. is deprecated."
and
"The font size slug should not have any special character. is deprecated."
deprecated() unconditionally appends is deprecated. to its first arg 😕 Seems like it might not be the right tool for the job after all? Should we go with plain old console.warn() instead?
Done that at 0b410ad |
ockham
approved these changes
Jun 16, 2021
ockham
reviewed
Jun 16, 2021
|
|
||
| // In the past, we used lodash's kebabCase to process slugs. | ||
| // By doing so, this method also stripped special characters | ||
| // such as the # in "#FFFFF". Some plugins relied on this behavior. |
There was a problem hiding this comment.
Looking at this again, what kind of values are we expecting here? 🤔 #FFFFFF would be a color value -- that really doesn't make a whole lot of sense for a font size, so we might not want to support that.
There was a problem hiding this comment.
Happy to update the comment, but I think we still need to provide this conversion: someone may have been doing $my_font_size.
|
@ockham and I looked into this and it looks like there are some cases we need to cover such as |
|
@nosolosw - did you want to keep this one open to circle back on this work, or will you open new PRs for the rework of this? Feel free to close it if not needed now. |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
With Gutenberg 10.8.1 block content fails to load in the editor if a number is passed to
getFontSizeClassinstead of a string. Previously lodash kebabCase was forgiving of this misdemeanor. This PR makes sure we have a string before running .replace to ensure that existing third party blocks that are passing a number continue to work.Fixes: #32719
To test
npm install && npm run build:plugin-zipand upload.Switch back to editor view and make sure the content displays, and that the following error does not show in the console. (there may be some other block invalidation issues - we are working on those separately)