Site Logo Block: Fix non-admin users seeing zero character #65010
Conversation
This file contains hidden or 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: +2 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
akasunil
approved these changes
Sep 3, 2024
Member
akasunil
left a comment
There was a problem hiding this comment.
Good catch! Works as expected. LGTM
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
approved these changes
Sep 3, 2024
ramonjd
reviewed
Sep 4, 2024
| } ); | ||
| const _isRequestingMediaItem = | ||
| _siteLogoId && | ||
| !! _siteLogoId && |
Member
There was a problem hiding this comment.
Great catch, thanks for spotting it 🥇 LGTM
Member
|
Going to merge this as it just happened to me! 
|
Contributor
Author
|
Thank you everyone for the reviews ❤️ |
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.
Follow-up #64919
What?
This PR prevents non-admins from seeing the string "0" in the Site Logo block if no logo is set.
Why?
This PR only contains a two-character change, but I'll explain why step by step.
Here we get the site data. If the site logo has not yet been applied and we are not an non-admin user,
siteData?.site_logowill have0:gutenberg/packages/block-library/src/site-logo/edit.js
Lines 416 to 418 in 18164f0
Next, we check whether the media is being retrieved.
_siteLogoIdis0, a falsy value. Due to the rules of logical operators, a short-circuit evaluation is performed and the value on the left-hand side is returned unchanged, so_isRequestingMediaItemhas a0value. Ideally, the left-hand side should be converted to a Boolean value here:gutenberg/packages/block-library/src/site-logo/edit.js
Lines 425 to 430 in 18164f0
As a result,
isLoadinghere will have the following formula. Since the left side does not evaluate to a match, the right side, which is0, is returned:gutenberg/packages/block-library/src/site-logo/edit.js
Line 538 in 18164f0
Here the spinner is rendered:
gutenberg/packages/block-library/src/site-logo/edit.js
Lines 659 to 665 in 18164f0
Since
isLoadingis0, it short-circuits and expands to this:How?
Since
_isRequestingMediaItemis expected to be a boolean value, I explicitly converted_siteLogoIdin the conditional expression to a boolean type. This problem was inherently there, but thankfully it was brought to light by #64919.Testing Instructions
0will not be displayed.