-
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
Site Editor: Append template type and name to the site editor page title #47855
Conversation
Size Change: +594 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
Flaky tests detected in 9426e5e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4128613144
|
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.
It tested well for me! Not sure if it satisfies the accessibility checklist though.
let titleBreadcrumb; | ||
if ( isReady && editedPost ) { | ||
const type = | ||
editedPostType === 'wp_template' | ||
? __( 'Template' ) | ||
: __( 'Template Part' ); | ||
titleBreadcrumb = `${ editedPost.title?.rendered } ‹ ${ type } ‹ `; | ||
} |
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.
Nit: I think can just construct the title
string here for simplicity (useMemo
is probably not needed):
let titleBreadcrumb; | |
if ( isReady && editedPost ) { | |
const type = | |
editedPostType === 'wp_template' | |
? __( 'Template' ) | |
: __( 'Template Part' ); | |
titleBreadcrumb = `${ editedPost.title?.rendered } ‹ ${ type } ‹ `; | |
} | |
const title = useMemo( () => { | |
const type = | |
editedPostType === 'wp_template' | |
? __( 'Template' ) | |
: __( 'Template Part' ); | |
return `${ editedPost.title?.rendered } ‹ ${ type } ‹ ${ __( 'Editor (beta)' ) }` | |
}, [ editedPost.title, editedPostType ] ); |
Then later we can just do:
useTitle( isReady && title );
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 was thinking that there might be instances where editedPost
was not set so title would then just need to be Editor (beta)
but I did some more testing and this doesn't seem to ever be the case, so have simplified as you suggested, but without the useMemo - this isn't a calculation heavy bit of logic so I agree that this probably isn't needed.
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.
Tested and confirmed the page title updates, this is a nice change. 👍
Cherry-picked this PR to the wp/6.2 branch. |
Nice! Thanks everyone ❤️ |
? __( 'Template' ) | ||
: __( 'Template Part' ); | ||
title = `${ editedPost.title?.rendered } ‹ ${ type } ‹ ${ __( | ||
'Editor (beta)' |
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 think we should remove "beta" now.
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.
Also, I'm not entirely certain that we should concat translated items like that, we should probably be using sprintf
here.
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.
There's also a hook useEditedEntityRecord
that would save a few lines of code here (allows to call getTitle
directly and also provide access to the record
if needed.
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 have added a follow up PR here to cover off these suggestions.
What?
Adds the template name and type to the page title
Why?
Currently every page in the editor has the same tile, which makes navigating the history impossible.
Fixes: #47730
How?
Passes the page template name and type into the
useTitle
hook.Testing Instructions
Open the site editor
Navigate to different templates and template parts
Check that the page title updates with the template type and name as you navigate, and that the browser history shows the full page titles
Screenshots or screencast
Before:
pagetitle-before.mp4
After:
page-title-after.mp4