Title Bar shifts when switching between saved and save#52365
Title Bar shifts when switching between saved and save#52365n2erjo00 wants to merge 9 commits intoWordPress:trunkfrom
Conversation
…g in case text changes
… found that it is actually being used
|
Thanks for the pull request. Unfortunately I don't think this solution will cover other languages where "save" and "saved" can be varying lengths. Did you have any other ideas? |
|
@jameskoster, maybe we should drop different label text; let the button's disabled state and snackbar communicate the saved state? |
|
Yes I considered that, but I seem to recall the label change being an a11y requirement. Perhaps @alexstine can confirm? Alex: would it be okay for the save button label to say "Save" (rather than "Saved") when the button is disabled? |
|
Agreed if we need to support every language version then button text cannot change. My (educated) guess would be that text changes because of accessibility. I think there was specific I think I have time time to do that upcoming weekend Edit: Then again changing aria-attributes or text might/is bit over-engineering. Toggling |
|
Preventing the text change is okay but you need to handle this with ARIA. Remember the basics, if sighted people see a loading state, it should be communicated to screen readers. The disabled attribute is not something that should be used here due to the focus issues. There is an experimental prop on the |
|
Thank you, @alexstine! The "Save" button already communicates its state using the ARIA attributes, just like the "Publish" button in the post editor. So I think we should be safe just to change the text. @n2erjo00, do you want to work on this? |
|
Yes I can 😁 |
|
What you think about the post editors Update button. It also has text change happening but it does not make header layout shift. Should we in the name of consistency use static text there too? Screen.Recording.2023-07-16.at.17.33.15.mov |
|
Possibly yes, but let's look into that one separately. For this PR, your implementation will also affect the button states when previewing a theme. It would probably be better to update |
… function returns static string for button text and dynamic text for aria-label
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
I think we can just drop the disabled label string from the getLabel helper. You don't need to refactor it into multiple helpers.
The label and button text should be the same.
Why
The label also renders the aria-label attribute, and it should match the text content of the button.
Usually, we would omit the label when a button has the text content, but it's needed here for the tooltip and consistency with other buttons.
Example
const getLabel = () => {
if ( isPreviewingTheme() ) {
if ( isSaving ) {
return __( 'Activating' );
} else if ( isDirty ) {
return __( 'Activate & Save' );
}
return __( 'Activate' );
}
if ( isSaving ) {
return __( 'Saving' );
} else if ( defaultLabel ) {
return defaultLabel;
}
return __( 'Save' );
};
|
Removed the "Saved" string from the component. Note: header layout shifts when user hits "Save" since the text changes. |
|
Hi @n2erjo00 are you available to update the pull request with the latest changes from trunk, and make sure that the final playwright tests run (and pass). |
|
@carolinan Yeah I am here. I'll do updates during this week |
|
@carolinan Updated to latest trunk. I don't really remember where this was left before it went stale but looking forward of community input and to see where it goes 😄 |
|
Hi @n2erjo00 - Unfortunately I was unable to follow up. Thank you for working on this issue and PR. The issue has been solved with a different pull request, so I will close this. |
5 participants
What?
Removed
Savedstring from components button text/aria-labelWhy?
Fixes #52295
How?
Testing Instructions
Check the original issue how to replicate the problem but observe now how button width remains the same and layout of header does not shift.
Testing Instructions for Keyboard
Screenshots or screencast