-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Comments
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. |
@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. |
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. |
Solid arguments all around 💯 I'll review your PR, thank you! Maybe a good GIF will help either confirm or weaken the principle! |
Hey how is the issues coming along @jasmussen and @stokesman ? Have a great weekend! |
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. |
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)
The text was updated successfully, but these errors were encountered: