Make usage of the settings icon more consistent #63020
Merged
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
|
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: +4.68 kB (+0.27%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
This was referenced Jul 1, 2024
ntsekouras
reviewed
Jul 2, 2024
| @@ -36,36 +36,4 @@ test.describe( 'isTyping', () => { | |||
| // Toolbar Popover is hidden again | |||
| await expect( blockToolbarPopover ).toBeHidden(); | |||
| } ); | |||
|
|
|||
| test( 'should not close the dropdown when typing in it', async ( { | |||
There was a problem hiding this comment.
@ellatrix do you think we should update this test to something like typing in the link dialog or something instead of removing it?
There was a problem hiding this comment.
This test should be kept for sure now, that you have reverted the Query Loop changes..
ntsekouras
reviewed
Jul 2, 2024
There was a problem hiding this comment.
Thanks for the PR! Code wise this looks good. It would be good to have some design input though, especially about the moved controls in inspector controls. --cc @WordPress/gutenberg-design
Additionally, if the change of icon in DataViews requires more discussion for some reason, we can always split this PR and land the Query Loop changes first.
t-hamano
reviewed
Jul 2, 2024
There was a problem hiding this comment.
Moves the Query Loop block 'Display settings' to the inspector.
Refactoring the Query Loop block controls feels like a secondary effort to avoid using icons.
#58207 has already been submitted for refactoring the Query Loop block controls, so can we just discuss the DataViews icon in this PR?
+1. I actually have it on my list to update the view options icon in data views (also to split the layout controls into a separate menu, but that's another story). The cog is what I had in mind, so this works well for me 👍 |
+1 to this too. I'd rather see that previous PR picked up again to ensure it aligns design wise. |
|
Thanks for pointing me at #58207 One of th problems in this project is PRs without an associated issue (which is required accordingly to the contributign guidelines) don't help contributors to follow all the fast-moving parts in this project. On the specifics od the design proposed in #58207 I will comment on that PR but I'm not sure I like hte idea of burying down into an ellispis menu such important controls. |
I perceived this comment on the issue as a green light to use the |
I agree for the most part, but there are some exceptional cases where a simple PR following up on another one shouldn't need an extra task; that would actually prevent the project from moving fast and create noise. I would be happy to update the contributing guidelines to reflect this point. |
Actually, this is not required, as the guidelines specify that only |
I know. That's why I stated that creating an issue is required accordingly to the contributing guidelines of this project (for non-trivial changes). A while ago I also researched the history of the project to read the conversation and arguments made when the guidelines were introduced in the documentation and I'd totally agree with those argumentations.
Sure, in that case the PR could just have a short notation 'Follow up to' with a link to the relevant issue. |
|
I reverted the changes to the Query loop block and fixed the typo I'll try to help #58207 move on. |
ntsekouras
approved these changes
Jul 4, 2024
|
Oh yes thanks. I also forgot to remove the unnecessary |
|
Added back the missing test and removed the redundant
I will update the testing instructions. |
t-hamano
approved these changes
Jul 4, 2024
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
* Move Query block Display settings from block toolbar to inspector. * Replace dataviews View options icon with the cog icon. * Remove no longer necessary test. * Revert changes to the Query loop block. * Fix typo. * Add back test. * Remove unnecessary BaseControl and fix aria-describedby. Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: priethor <priethor@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: mtias <matveb@git.wordpress.org>
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 #62873
What?
This PR only aims to make the usage of the
settingsicon more consistent in the editor UI. This icon should only be used to switch a control to an alternative version, see for example the various margin, padding, spacing controls.Instead, it was used inconsistently in at least 3 other cases:
Screenshot of the icon, for reference:
Work on this PR surfaced other issues. Most notably, the Query loop block had the 'Display settings' placed in the toolbar, while it makes more sense to have them in the Inspector together with other similar settings.
Other findings will be tracked in new issues.
Why?
Icons should uniquely identify specific features or, at least, similar features.
There is already an icon for the concept of 'settings': the
cogicon is commonly used for this purpose. It makes sense to reuse popular conventions to make the editor UI more intuitive and less disorienting.How?
BaseControls that were wrapping theNumberControls. Not sure why these wrappingBaseControls were used in teh first place but they seem unnecessary. Also, the description of the 'Max pages to show' setting was incorrectly set to the wrappingBaseControlinstead of the actualNumberControl.icon to use thecog` icon.Testing Instructions
Old testing instructions
aria-describedbyand ID attributes.cogicon.New testing instructions
cogicon.Testing Instructions for Keyboard
Screenshots or screencast
The Query loop block settings moved to the Inspector:
The new cog icon in the data views: