Remove right negative margin from pinned items #57666
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
t-hamano
added
[Type] Enhancement
|
Size Change: -32 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
@jasmussen will know for sure but I think this style may have been added to visually balance the position of the icons accounting for the minimal Options (ellipsis) icon. For me the inconsistent spacing between these buttons is equally awkward, so I would approve of this change because it simplifies things a little. But let's see what Joen says. |
|
Yes that's right. The vertical ellipsis due to the thinness of the icon is better optically balanced with the negative margin. We used the negative margin instead of a smaller width, to keep the touchable footprint of the ellipsis the same size. I wonder if there's a better way to document this aspect of the vertical ellipsis button. |
|
I'm not sure how sustainable it is to apply that convention holistically. For instance in the data views we have actions at the end of each table row, with the ellipsis serving as an 'overflow menu': 
If we applied the negative margin here then the action icons lose alignment with the pagination buttons. There's a similar story in the Site Editor when the Styles panel is open. Note the slight mis-alignment of icons: 
I'm not sure this trade-off is really worth it given the maintenance burden. I'd be inclined to opt for a more consistent convention, e.g. make all 'toolbar' icon buttons 32px, with 8px gap: 
Then see if we can balance the ellipsis another way. Perhaps try the horizontal orientation, or dare I say a slightly heavier stroke? Alternatively the negative margin should probably be built into the component somehow, so that it doesn't need to be applied manually across all icon buttons that use the vertical ellipsis. |
|
I do agree there's potential for simplification here. What if the ellipsis button was always 24x32? Or what if the ellipsis button had a |
|
@jasmussen @jameskoster Thank you for your wonderful review.
It will look something like this: Narrowed the button width from 32px to 24px and removed the negative margin on the left element. Furthermore, since the Option button appears to be closer to the right side, I added 4px to the padding on the right side. Personally, I find this style a little cramped 😅
|
Feels cramped to me only when focused, whereas the resting state which you'd see most of the time feels off balance. Would the negative left/right margins on the ellipsis have any side effects? Or does it have to be a negative left margin on the ellipsis, since presumably the ellipsis is always last in a series? |
|
The only method I can think of is to add a style to the button component that applies the negative margin to the last button in a series when the icon is an ellipsis. But that's not bullet-proof because an ellipsis could appear next to a group of buttons in a container, in which case the negative margin wouldn't be applied. It seems very tricky to write a style that will account for all situations, but if there's a smarter way to build this behavior into the component then it would be good to explore. Otherwise it doesn't feel worth the hassle to me. There will always be circumstances where the negative margin isn't applied which creates inconsistency. |
|
Personally, I'm hesitant to put negative margin on the The main reason I submitted this PR is that when I enable "Show button text labels", the spaces between the buttons look very cramped.
For example, how about an approach that overrides the current negative margin to zero only when "Show button text labels" is enabled? |
I agree. Building the behavior into the component is preferable, but only if it can be implemented with reliable performance – unfortunately I don't think it can. So we're left needing these situational, manually-added negative margins where ellipsis buttons are used, which is not ideal from a maintenance perspective (see this whole discussion :D), and inevitably leads to inconsistencies. It's not a strong feeling but I would still lean slightly towards merging this because it simplifies things, and because the benefit of the visual balance seems to be lost when the plugin adjacent to the ellipsis is selected: 
I don't love adding more situational styles to cater to a specific preference, but it could be a compromise if there are strong feelings about keeping the negative margin. |
|
It seems like there are some strong preferences for being generic and not having exceptions, I think @richtabor also stumbled on this, so I'm happy to defer to you all on this. |
jameskoster
approved these changes
Jan 11, 2024
|
@jasmussen @jameskoster Thank you for your deep insight. I would like to merge this PR, but there may be an opportunity to reconsider this negative margin in the future. |
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?
This PR removes the negative margin on the right from pinned items in the header. This should ensure that all icons have the same gap of 8px.
Why?
This style was added in #40411 to fix gaps, but now it seems unnecessary.
Testing Instructions
Try a scenario like the one below.
To test Custom Pinner Item, run the code below in your browser console.
Post Editor:
Site Editor:
Screenshots or screencast