-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block Theme Preview: Display the theme name on the activate button #55752
Block Theme Preview: Display the theme name on the activate button #55752
Conversation
20233de
to
53d02da
Compare
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 looked good to me! 💯 Thanks for this improvement!
isDirty: dirtyEntityRecords.length > 0, | ||
isSaving: | ||
dirtyEntityRecords.some( ( record ) => | ||
isSavingEntityRecord( record.kind, record.name, record.key ) |
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 seems to be a prettier error on this line.
https://github.com/WordPress/gutenberg/actions/runs/6730275641/job/18292658610?pr=55752#step:5:592
Replace `·record.kind,·record.name,·record.key·` with `⏎↹↹↹↹↹↹↹record.kind,⏎↹↹↹↹↹↹↹record.name,⏎↹↹↹↹↹↹↹record.key⏎↹↹↹↹↹↹` prettier/prettier
I don't think this has anything to do with this PR change, but it would be good to address it before merging! @arthur791004
I'm sorry for my late review.
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.
Updated!
6e32c7e
to
0f46f65
Compare
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've not tested this but I approve of the direction
return sprintf( 'Activating %s', previewingThemeName ); | ||
} else if ( disabled ) { | ||
return __( 'Saved' ); | ||
} else if ( isDirty ) { | ||
return __( 'Activate & Save' ); | ||
return sprintf( 'Activate %s & Save', previewingThemeName ); | ||
} | ||
return __( 'Activate' ); | ||
return sprintf( 'Activate %s', previewingThemeName ); |
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.
With this PR, these strings are no longer translated. This needs to be fixed, @arthur791004.
Since the new strings involve interpolation, they require // translators:
notes.
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.
Oops...
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.
Here is the fix: #57147
What?
We want to display the name of the previewing theme on the “Activate” button to make it clearer what theme you're previewing.
Note that I keep the text of the “Activate” button on both “Save modal” and “Save panel” unchanged as
Does it make sense? Or do we still want to display the theme name there?
Why?
Resolve #52772
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast