search

vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
473 stars 83 forks source link

[lumo] Do not use border-radius properties to define margin #3407

Open web-padawan opened 2 years ago

web-padawan commented 2 years ago

Motivation

Currently --lumo-border-radius-* properties are used in some rather unexpected places.

https://github.com/vaadin/web-components/blob/67b8a7ce8d12ef24d8208c41be1f616fc51ccb60/packages/vaadin-lumo-styles/mixins/helper.js#L24

https://github.com/vaadin/web-components/blob/67b8a7ce8d12ef24d8208c41be1f616fc51ccb60/packages/vaadin-lumo-styles/mixins/required-field.js#L18

https://github.com/vaadin/web-components/blob/67b8a7ce8d12ef24d8208c41be1f616fc51ccb60/packages/form-layout/theme/lumo/vaadin-form-item-styles.js#L21

https://github.com/vaadin/web-components/blob/67b8a7ce8d12ef24d8208c41be1f616fc51ccb60/packages/list-box/theme/lumo/vaadin-list-box-styles.js#L19

https://github.com/vaadin/web-components/blob/67b8a7ce8d12ef24d8208c41be1f616fc51ccb60/packages/vaadin-lumo-styles/typography.js#L116

Expected outcome

Changing the custom CSS property only changes border radius.

Actual outcome

Changing from em to rem caused many screenshots to fail unexpectedly https://github.com/vaadin/web-components/pull/3405#issuecomment-1031388907

jouni commented 2 years ago

I’m currently in favor of removing this complexity.

The reason for it initially is to improve perceived visual alignment (the edges of the label and the input field), but now I think it’s not worth it. And it also causes visual alignment issues with anything else outside the field itself, so it might even make things look worse, especially if you use a very large radius.

rolfsmeds commented 2 years ago

What if we just introduced a property (or several) for easily overriding this instead of changing the default?

jouni commented 2 years ago

I don't think it would be worth it. If some designer/developer wants to address the same issue as I originally tried, they could target the corresponding parts directly with ::part(label) etc. and adjust the margin/padding as needed.

rolfsmeds commented 2 years ago

Ah, right, I only now noticed that those styles mainly affect labels (and helpers and dividers) – not paddings inside fields.

So, yes, in favor of refactoring to appropriate spacing properties in next major (and of course providing instructions on reverting the change in the Upgrading Guide).