Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BorderBoxControl: Reduce gap value when unlinked #67049

Merged
merged 6 commits into from
Nov 29, 2024

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Nov 16, 2024

Fixes #65207

What?

This PR narrows the width of the input field so that the BorderBoxControl component doesn't overflow the block sidebar.

This PR changes the gap of the BorderBoxControl component from 16px to 12px to prevent horizontal scrolling in block sidebars.

Why?

When a block sidebar has a scrollbar, the width of the container in which the BorderBoxControl component is rendered is 231px on Windows and 233px on MacOS.

On the other hand, the BorderBoxControl component has a width of 248px (Input field 116px + gap 16px + input field 116px). This width is larger than the container width on both Windows and MacOS.

How?

Change the input field width to 104px. This makes the entire component width smaller than the block sidebar container width.

Change the Gap value from 16px to 12px. The BorderBoxControl component still overflows its container, but the horizontal scrollbar no longer appears.

Testing Instructions

  • Insert a Paragraph block.
  • Open the Border setting from the Dimensions panel.
  • Click the Unlink sides button.

Screenshots or screencast

In my environment (Windows OS), the number of digits that can be displayed changes from 4 to 3, but I'm sure there are very few users who want to apply a 4-digit border.

Before After
image image

@t-hamano t-hamano self-assigned this Nov 16, 2024
@t-hamano
Copy link
Contributor Author

I am wondering whether we should also change the width when linked:

width={
size === '__unstable-large' ? '116px' : '110px'
}

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Nov 16, 2024
@t-hamano t-hamano marked this pull request as ready for review November 16, 2024 10:53
@t-hamano t-hamano requested a review from a team November 16, 2024 10:53
Copy link

github-actions bot commented Nov 16, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

github-actions bot commented Nov 16, 2024

Flaky tests detected in ceb266a.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12084951443
📝 Reported issues:

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look too good on MacOS with even 3-digit numbers for borders:

Screenshot 2024-11-18 at 16 32 23

Increasing it to 108px seems to do the job for me:

Screenshot 2024-11-18 at 16 32 35

I wonder if that would be an acceptable compromise on Windows as well?

@t-hamano
Copy link
Contributor Author

Thanks for the review!

Increasing it to 108px seems to do the job for me:
I wonder if that would be an acceptable compromise on Windows as well?

At 108px, it will overflow the container width of 231px by 1px on Windows (Input field 108px + gap 16px + input field 108px = 232px).

This might be acceptable, but what do you think?

231px

Another thing I noticed is that when I select the rem unit, the input field becomes narrower:

trunk (116px) This PR (108px)
image image

Personally, I think few users will want to use a radius larger than 100px, so as long as two digits are always visible, I think it's acceptable.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about this solution, since we're chasing ghosts.

Would it be better if we just reduce the gap prop from 4 to 3? That would reduce the gap between the left and right control groups, and maybe fix the overflow issue, which is the primary reason for this PR?

@t-hamano
Copy link
Contributor Author

Would it be better if we just reduce the gap prop from 4 to 3? That would reduce the gap between the left and right control groups, and maybe fix the overflow issue, which is the primary reason for this PR?

This approach prevents the horizontal scrollbar from appearing:

image

But the component is overflowing the container width:

image

Maybe this is a good approach for now 🤔

@tyxla
Copy link
Member

tyxla commented Nov 20, 2024

I'm personally OK with this. Should we ping @WordPress/gutenberg-design just in case?

@tyxla tyxla requested a review from a team November 20, 2024 09:46
@jasmussen
Copy link
Contributor

I think this one is fine to ship without changing gaps, as fixing the horizontal scrollbar is the most important. Text inputs are organically scrolling horizontally, for example you might have a default address input field where a very very long irish address might require you to use the right hand arrow-key to navigate to the end of it. What if someone wanted a 999999px border? We can only optimize for the 80% use case, and thankfully base inputs handle the rest.

What I do think we can do separately, is consider changing the default width of the inspector from 280px to potentially 284px. I also think the "rem" unit selector button can become square in aspect ratio; that might mean the 2 letter rem text might have very little padding, but since it's a text only button, that shouldn't be too big a problem. But as a unit select, I don't expect it to change size between selections, just like I wouldn't expect a select control to change sizes between selected options. A -1px letter spacing option could even be explored for the unit itself.

@t-hamano t-hamano changed the title BorderBoxControl: Reduce input field width when unlinked BorderBoxControl: Reduce gap value when unlinked Nov 20, 2024
@t-hamano
Copy link
Contributor Author

In this PR, I adopted the approach of changing the gap from 4 (16px) to 3 (12px). I also updated the PR description and screenshots.

What I do think we can do separately, is consider changing the default width of the inspector from 280px to potentially 284px.

This approach also works.

In fact, it only takes a 1px adjustment somewhere to make the horizontal scrollbar disappear 😅 I think we have two options: slightly increase the width of the sidebar or slightly decrease the gap of the BorderBoxControl or the width of the input field.

0e8a452468d050e67c9f4daba641b7e6.mp4

@jasmussen
Copy link
Contributor

I'm happy with either, or both, including making the individual inputs not quite as wide.

@t-hamano
Copy link
Contributor Author

What do you think about merging this PR?

This PR simply reduces the gap value from 16px to 12px. In fact, it overflows the container width, but the horizontal scrollbar never appears.

@tyxla
Copy link
Member

tyxla commented Nov 29, 2024

I'm personally good with merging it 👍 Feel free to ship, given we have a design approval on it.

@t-hamano t-hamano merged commit f189eab into trunk Nov 29, 2024
63 checks passed
@t-hamano t-hamano deleted the border-box-control-width branch November 29, 2024 14:02
@github-actions github-actions bot added this to the Gutenberg 19.9 milestone Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BorderBoxControl: Horizontal scrollbar is displayed in block sidebar
3 participants