Makes spacing consistent in zoom out vertical toolbar #63994
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
amitraj2203
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: +23 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a69454f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11156023201
|
richtabor
reviewed
Jul 31, 2024
richtabor
reviewed
Jul 31, 2024
|
Hi @richtabor, Is there something that needs to be done from my side? |
getdave
reviewed
Sep 16, 2024
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
getdave
reviewed
Sep 26, 2024
There was a problem hiding this comment.
I noticed the changes were previously in the @wordpress/components package which shouldn't have any awareness of "zoom out".
I therefore moved the CSS to the @wordpress/block-editor package which is where the state for "zoom out" resides.
However, this change hasn't worked implementation wise so we'll need to look at this again if possible 🙏
The aim is to get consistent spacing between each of the toolbar icons.
richtabor
reviewed
Sep 30, 2024
|
|
||
| // Make the spacing consistent between controls. | ||
| .components-button { | ||
| height: 40px; |
There was a problem hiding this comment.
Any reason this shouldn't use $button-size-next-default-40px?
There was a problem hiding this comment.
Suggested change
| height: 40px; | |
| height: $button-size-next-default-40px; |
@richtabor Not as far as I know. However, when I checked out this PR the implementation doesn't quite work.
I would look to fix this, but I actually find it really hard to notice the differences/inconsistencies in spacing. My brain plays tricks.
Moreover, sometimes even when the spacing between items is "correct" it still looks visually "off" because of the relative space occupied by the icons in use. I wonder whether we'll need a default spacing and then custom / specific spacing on a case by case basis?
|
Nice. Makes them feel more like one unit of control for the pattern. Before
After
|
draganescu
force-pushed
the
fix/issue-63807
branch
from
c270065 to
dd9ff72
Compare
draganescu
approved these changes
Oct 1, 2024
There was a problem hiding this comment.
I think this is OK, for me using the scss variable is the same as using the number itself. I don't think the styling should be elsewhere as it styles stuff in the block editor package. What am I missing @getdave ?
|
@draganescu This is now all good. I moved the CSS into the correct package in previous commits and @richtabor has confirmed he's happy with the visual appearance (which I was struggling to verify myself). Therefore I'd be happy to merge this. |
|
Needs a rebase to allow tests to pass |
getdave
force-pushed
the
fix/issue-63807
branch
from
a69454f to
8f655fa
Compare
|
👋 Hello! I would ask you to revisit the approach taken here:
Those are best practices that can go a long way in raising the quality of our code and our UIs; they are something that we hope everyone working on Gutenberg can learn and apply both when authoring and when reviewing code. cc @WordPress/gutenberg-components @WordPress/gutenberg-design |
|
Ok thanks @ciampo. I'll take ownership of sorting this out as I should definitely have known better. Can I ask is any of the above in public documentation that you would be able to point me towards? |
Thank you, @getdave , I appreciate!
This paragraph specifies that "A component's class name should never be used outside its own folder" and highlights how this is a best practice to ensure encapsulation and reusability. I don't believe the rest is really codified in public docs, I'd say it's more common sense. But there should definitely be an effort from our side to write better docs, best practices, maybe even lint rules. |
|
I'm thinking more along the lines of https://make.wordpress.org/core/2024/09/12/gutenberg-development-practices-and-common-pitfalls/. One person's "common sense" is often not that clear to others and thus I"m keen we start to surface these "best practices" or "common sense guidelines". I'm not attempting to absolve myself here, but rather noticing that newer contributors may not share the context that you and I (should) have. |
|
You're absolutely right, I'll start brainstorming a similar post and share it with y'all at a later point |
getdave
added a commit
that referenced
this pull request
Oct 3, 2024
|
Follow up here #65858
Feel free to share a draft and I'll happily proof read. |
getdave
added a commit
that referenced
this pull request
Oct 4, 2024
See #63994 (comment) Co-authored-by: getdave <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: ciampo <[email protected]>
noisysocks
added
the
Backported to WP Core
noisysocks
pushed a commit
that referenced
this pull request
Oct 6, 2024
* chore: Update block vertical toolbar height in variables and styles * Removed the variable and hardcoded the value. * Avoid changes to components package * use the button width scss variable --------- Co-authored-by: amitraj2203 <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: richtabor <[email protected]>
noisysocks
pushed a commit
that referenced
this pull request
Oct 6, 2024
See #63994 (comment) Co-authored-by: getdave <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: ciampo <[email protected]>
peterwilsoncc
pushed a commit
to peterwilsoncc/gutenberg-build
that referenced
this pull request
Oct 6, 2024
See WordPress/gutenberg#63994 (comment) Co-authored-by: getdave <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: ciampo <[email protected]> Source: WordPress/gutenberg@18fdd25
peterwilsoncc
pushed a commit
to peterwilsoncc/gutenberg-build
that referenced
this pull request
Oct 6, 2024
See WordPress/gutenberg#63994 (comment) Co-authored-by: getdave <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: ciampo <[email protected]> Source: WordPress/gutenberg@86e278c
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
See WordPress#63994 (comment) Co-authored-by: getdave <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: ciampo <[email protected]>
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.
What?
Fixes: #63807
Why?
See this comment
Testing Instructions
Screenshots or screencast
Co-authored-by: amitraj2203 [email protected]
Co-authored-by: getdave [email protected]
Co-authored-by: draganescu [email protected]
Co-authored-by: richtabor [email protected]