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

Buttons Block: Border-radius should start unset #29210

Closed
jasmussen opened this issue Feb 22, 2021 · 6 comments · Fixed by #31483
Closed

Buttons Block: Border-radius should start unset #29210

jasmussen opened this issue Feb 22, 2021 · 6 comments · Fixed by #31483
Assignees
Labels
[Block] Buttons Affects the Buttons Block [Status] In Progress Tracking issues with work in progress

Comments

@jasmussen
Copy link
Contributor

Description

When you insert a button in the buttons block, it starts with a pill-shape. But the inspector shows the radius set to "5px", which is inaccurate.

Step-by-step reproduction instructions

Insert a button, then carefully change the radius from 5px to for example 6px, and observe the big jump.

Expected behaviour

I would expect the border radius to be unset, because that's what it is. The field could be empty.

Screenshots or screen recording (optional)

button radius

@jasmussen jasmussen added the [Block] Buttons Affects the Buttons Block label Feb 22, 2021
@stokesman
Copy link
Contributor

I agree that showing no value would be better than an incorrect value. Apparently, that was the behavior when #17596 was opened. Technically, it is correct, the value is unset. Maybe that is the way to go. Though coincidently, some hours before this issue opened I made #29193. It aims to meet the (more naive) expectation that the value would reflect the actual button style. It works well but I was considering adding some coverage of an edge case and that's why it's still drafted. Your feedback is most welcome @jasmussen.

@jasmussen
Copy link
Contributor Author

@stokesman your contributions have been stellar, so far, thank you so much.

I can definitely understand the desire for the sidebar control to reflect what you see, but in situations like these (and with #27331 in ther corner of my eye), I tend to always look for generically applicable solutions, heuristics that would apply across any control or block or combination thereof.

In this case, the pill-shape and square shapes are style variations that both happen to touch the border radius, but theoretically a theme could have overridden them to change other properties of the button itself, including the default border radii. In both cases, the border radius is supplied by a stylesheet. Whereas the border radius in the sidebar, once tinkered with, will provide an inline style to override whatever rule was set by a stylesheet. So it is "unset", and showing "5" or any other best guess would be misleading regardless of the accuracy of the guess. I sense you fundamentally agree, but figured I'd expand a bit on this.

We will encounter this again for most other new controls that we add, and allowing them to be unset scales, whereas I worry that best guesses, despite best intentions, will only add complexity and confusion.

@stokesman
Copy link
Contributor

I don't have a dog in this fight… I have two #29263

Though I would like to point out that the approach in #29193 doesn't make guesses. The accuracy is 💯 but that's not to say it's without limitation. Primarily that the value is always in pixels. Still, like you pointed out, it is technically misleading. The value is not set.

With consideration for consistency, I want to mention the opacity slider in the Cover block. The initial value shows 50 but it too is not actually set, just “guessing” what the provided style is (which a theme could override). For that case, rather than showing an empty value, it might be preferable that the opacity (and the color when an image is picked before a color) could default to actual values stored in block attributes.

@jasmussen
Copy link
Contributor Author

Solid arguments all around 💯

I'll review your PR, thank you! Maybe a good GIF will help either confirm or weaken the principle!

@paaljoachim
Copy link
Contributor

paaljoachim commented May 21, 2021

Hey how is the issues coming along @jasmussen and @stokesman ?
This is a bug which I look forward to getting fixed. Thanks!

Have a great weekend!

@stokesman stokesman linked a pull request May 22, 2021 that will close this issue
7 tasks
@stokesman
Copy link
Contributor

Hi @paaljoachim, at present we've got a PR I've made for this #29263 that works well enough, but I've gone ahead and linked #31483 as well because it happens to fix this too. I'm favoring the latter PR since it progresses the border support feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block [Status] In Progress Tracking issues with work in progress
Projects
None yet
3 participants