vaadin-component-factory / vcf-pdf-viewer-flow

Vaadin Addon for providing pdf viewing functionality
Apache License 2.0
8 stars 6 forks source link

Vaadin 24 - button border radius style is not applied because of removed variable --lumo-border-radius #33

Closed paodb closed 1 year ago

paodb commented 1 year ago

When using the component in a Vaadin 24 application, the button border radius style is not applied as it depends on a lumo variable that was removed.

Vaadin 24 button looks like:

pdf-toggle-button-v24

Should look as in previous versions:

pdf-toggle-button-v23

Frettman commented 1 year ago

--lumo-border-radius was deprecated and the "new" name has been --lumo-border-radius-m for quite a while now. So no fallback necessary.

javier-godoy commented 1 year ago

The approach was chosen in order to preserve compatibility with Vaadin 22-24. Version 2.7 of this add-on is compatible with "Vaadin 22+", and so far this is the only issue when using the add-on with Vaadin 24. Replacing --lumo-border-radius with --lumo-border-radius-m is a breaking change, and it would require releasing (and maintaining) a new major version of the add-on just for a small CSS change.

With Vaadin 22-23, button border-radius styles are taken from --lumo-border-radius. When the add-on is used with Vaadin 24, --lumo-border-radius is undefined and border-radius styles then fallback to --lumo-border-radius-s (which has the same value as --lumo-border-radius in Vaadin 22-23).

 border-radius: var(--lumo-border-radius, var(--lumo-border-radius-s));
Frettman commented 1 year ago

I'm pretty sure --lumo-border-radius-m already existed in Vaadin 22 (doc confirms it). But to be sure a fallback is fine of course. I mainly commented because I saw the commit and according to the docs --lumo-border-radius-m is the replacement for --lumo-border-radius. Huh, but --lumo-border-radius-s seems to have the same value as --lumo-border-radius-m, so it visually makes no difference for now. Wierd. https://vaadin.com/docs/latest/styling/lumo/lumo-style-properties/shape

paodb commented 1 year ago

Thanks for pointing this out, I think you're right, the documentation recommends the use of --lumo-border-radius-m as replacement so I will update the fallback value.