Navigation: Use getTitle to get the navigation title #52635
Closed
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
scruffian
added
[Type] Bug
|
Size Change: +76 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
getdave
reviewed
Jul 14, 2023
Contributor
There was a problem hiding this comment.
Seems like a good solution for now.
I wonder if in the future we should have a selector for getEntityTitle( id ) which does this? It's so common and it causes so many bugs all the time.
I think you should propose it.
Member
There was a problem hiding this comment.
This doesn't do anything different internally. See https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-site/src/components/use-edited-entity-record/index.js#L59.
Plus useEditedEntityRecord performs extra checks that aren't needed here.
I think we just need to make sure entities return values like titles in a predictable way.
Contributor
There was a problem hiding this comment.
Oh really. Got to be honest I was doing a fly by review. My comment probably wasn't super helpful.
My thought was we need a single source of truth for getting a string-based title. I think the raw vs rendered thing throws people. Maybe if we had some well-named utilities that got the correct title based on where you want to output it that night help.
getEntityTitleForField vs getEntityTitle or something. Just an idea...
Contributor
Author
There was a problem hiding this comment.
I updated this to just use getEditedEntityRecord. I'm not sure what the best way to do this is, but right now we have a JS error in trunk and this branch fixes it, so I'd rather get this merged and iterate on improvements later.
Member
There was a problem hiding this comment.
I’ll be able to do another round of reviews tomorrow morning.
Can you post more details on how to reproduce issue issue you’re having on trunk?
Contributor
Author
There was a problem hiding this comment.
I can't get it to happen since #52707 merged. Still investigating...
getdave
reviewed
Jul 17, 2023
...-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list.js
Outdated
Show resolved
Hide resolved
scruffian
force-pushed
the
fix/navigation-title-2
branch
from
7dc6aec to
ec1d0f7
Compare
scruffian
commented
Jul 17, 2023
...-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list.js
Outdated
Show resolved
Hide resolved
…ttern/template-part-navigation-menu-list.js
scruffian
commented
Jul 17, 2023
...-site/src/components/sidebar-navigation-screen-pattern/template-part-navigation-menu-list.js
Outdated
Show resolved
Hide resolved
…ttern/template-part-navigation-menu-list.js
Mamaduka
reviewed
Jul 18, 2023
Comment on lines
+16
to
+29
| const title = useSelect( ( select ) => { | ||
| const { getEditedEntityRecord } = select( coreStore ); | ||
| const { __experimentalGetTemplateInfo: getTemplateInfo } = | ||
| select( editorStore ); | ||
|
|
||
| const _record = getEditedEntityRecord( | ||
| 'postType', | ||
| 'wp_navigation', | ||
| id | ||
| ); | ||
|
|
||
| const templateInfo = getTemplateInfo( _record ); | ||
| return templateInfo?.title || __( '(no title)' ); | ||
| } ); |
Member
There was a problem hiding this comment.
Can we just use record?.title?.rendered || record?.title here? We can leave no title fallback outside of the mapSelect.
Using the selector getTemplateInfo meant for different entities can have unexpected results in the future.
P.S. The useSelect is missing a dependency here.
Mamaduka
reviewed
Jul 18, 2023
| return ( | ||
| <ItemGroup className="edit-site-sidebar-navigation-screen-template-part-navigation-menu-list"> | ||
| { menus.map( ( menuId ) => ( | ||
| { [ ...new Set( menus ) ].map( ( menuId ) => ( |
Member
There was a problem hiding this comment.
Is this change related to the title rendering fix?
If we need to ensure that menus are a unique set, then this probably has to be done higher in the tree, maybe in useNavigationMenuContent or getBlocksOfTypeFromBlocks?
Edit: I just saw the #52707. I think this can be removed here.
Contributor
There was a problem hiding this comment.
Yes I don't see a new for Set here unless I missed something...?
Contributor
Author
|
There's a proper fix here: #52758 |
Contributor
|
The issue this was solving is closed by #52758 |
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?
Gets the entity title in a more predictable format. An alternative to #52167.
Why?
Sometimes it seems like
useEntityPropreturns an object and sometimes a string, so sometimes this throws an error.How?
This changes the approach to use
getTitlewhich is more predictable.Testing Instructions