Refactor navigation title usage #52807
Merged
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
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
jeryj
force-pushed
the
refactor/navigation-title-usage
branch
2 times, most recently
from
adeb242
to
ea4afb5
Compare
|
Size Change: 0 B Total Size: 1.44 MB ℹ️ View Unchanged
|
|
Flaky tests detected in ea4afb5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5614667837
|
#52758 fixes the response from the fallback-navigation endpoint that did not have title.raw in the response, meaning we had to sometimes rely on title.rendered. This removes the extra 'or' statements that were added as hot fixes. The ones that still rely on title.rendered are due to the value needing to be checked within an object, or somewhere that the title property is not rendered via react.
getdave
force-pushed
the
refactor/navigation-title-usage
branch
from
18ae4d6
to
7ed9723
Compare
getdave
approved these changes
Jul 26, 2023
There was a problem hiding this comment.
Thanks for this tidying up and following through with Code Quality 👏
I've rebased this and also tested it across the places where you've made the fixes. I couldn't find any problems.
As you're in the weeds of this, I'd really appreciate an explanation of how it's possible to use just title without having to check for the sub fields. Brief overview of raw vs rendered would also be great. I used to know it well but I've forgotten it and I'm sure you will have it front of mind 🙇
Given that the PHP got backported to 6.3 RC does this PR also needs be backported or will it work without this fix?
If it needs backporting you will need the label and to ping one of the Core Editor Tech release leads.
I'm not 100% sure, but it looked to me that when interacting with the data object itself, you need to check for the
This is only a code quality refactor and doesn't fix any functionality, so it doesn't need to be backported. The old code will work fine, it just won't hit the unnecessary OR statements that this PR removed. |
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.
What?
#52758 fixes the response from the fallback-navigation endpoint that did not have title.raw in the response, meaning we had to sometimes rely on title.rendered. This removes the extra 'or' statements that were added as hot fixes. The ones that still rely on title.rendered are due to the value needing to be checked within an object, or somewhere that the title property is not rendered via react.
Why?
Code cleanup.
How?
Don't check for both
title.renderedandtitleprops.Testing Instructions
This is a trickier one to test. We will hopefully catch all the scenarios through manual testing. If some pop-up, which isn't unlikely, we'll need to add test coverage for it.