Improve the EntitiesSavedStates modal dialog design and labeling #67792
+162
−71
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
afercia
added
[Type] Bug
|
Cc @jameskoster @WordPress/gutenberg-core |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +305 B (+0.02%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
I can't comment on the code but appreciate the effort to tidy this up. The modal already looks much better. One drawback I noticed is that when there are many changes (or the viewport is small, or a combination thereof) the buttons disappear until you scroll: 
There's probably a broader question to ask about whether actions should be sticky across all modals, but I mention it here because despite the other improvements this is a slight downgrade in the UX. |
Yes, that's a feature / limitation of the Modal component. A potential change to introduce a sticky footer should be considered at the component's level. |
|
@jameskoster any other style refining you would like to see implemented? |
afercia
force-pushed
the
fix/refine-save-modal-dialog-within-dialog
branch
from
5cc1825 to
fb435f8
Compare
|
Flaky tests detected in fb435f8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12273581708
|
|
Agree that's a detail to work out at the component level, and I don't suppose it needs to be a blocker, though I'd welcome feedback from @WordPress/gutenberg-design on that detail. There's something about the overall spacing that works less well in this context. I wonder if it would be worth trying the
What do you think? |
It does seem a bit loose on the spacing, but it's unclear that needs to be a blocker. Probably worth getting this into the plugin, and gathering feedback through that channel, and then spacing polish can happen separately at any time, whether locally or component level. |
|
Re: the
It's a one-line change, I'd agree it can happen at any time if need be. |
|
For me the small modal works better, just because it feels more like the UI we're familiar with. Not a blocker though. As long as we use one of the defined modal sizes I'm happy. |
Sure. In the latest commit I made the modal dialog small so that everyone can try it and see how it looks on different viewport sizes. Cc @WordPress/gutenberg-design |
|
I agree with keeping the same spacing approach consistent with its version in the site editor. Regarding follow-up improvements, I see room for improvement in adding an indentation level in the bullet points and reducing the spacing between the checkbox and the bullet list. If I understand this correctly, more checkboxes can show up and placing the bullet list at the control label could improve the text relationship between what users are about to update and what that includes. This suggestion is for both modal and the inspector panel. |
|
Good point about the indentation. I'd like to see the list items text aligned with the checkbox label, with the bullets following the principle of the hanging punctuation:
|
afercia
force-pushed
the
fix/refine-save-modal-dialog-within-dialog
branch
from
38c8ef8 to
6a5e038
Compare
|
Latest commit adjusts the top and left margin of the changes list:
|
afercia
force-pushed
the
fix/refine-save-modal-dialog-within-dialog
branch
from
6a5e038 to
cd71c4b
Compare
|
Since we're discussing refinements, I have two questions:
|
|
The screenshot looks good to me 👍
Wow. TIL there was a word for that alignment. Thanks |
|
Sorry. My previous comment was on this message. It seems we pressed the "comment" button almost at the same time. Regarding your suggestions for font size and text color, I second your rationale and agree with the changes. |
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.
Fixes #49832
Fixes #67354
Note: if this gets merged, please add props to @dhruvang21 for the alternative PR #67473
What?
The modal dialog to save entities reuses the
EntitiesSavedStatescomponent 'as is'. This isn't ideal in terms oc content order, styling, and accessibility because modal dialogs expecte their content in a more meaningful order, with a visible title, specific styling and appropriate labeling and description.Why?
How?
isWithinModalDialogprop toEntitiesSavedStatesand its variant to distinguish when this component is used within a modal dialog.Testing Instructions
Saving your changes will change your active theme from Twenty Twenty-Four to Twenty Twenty-Five.There are 2 site changes waiting to be saved.Are you ready to save?Screenshot to illustrate the labeling / description:
Testing Instructions for Keyboard
Screenshots or screencast