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 progress bar height is non-integer #492

Open platosha opened 6 years ago

platosha commented 6 years ago

Lumo’s height for progress-bar is calc(var(--lumo-size-l) / 10);, which is 0.275rem, (4.4px with default font size of 16px).

Non-integer resulting height is causing rendering artifacts. Screenshot of Lumo demo from Chrome, note the different bar heights:

screen shot 2018-03-05 at 14 02 54

web-padawan commented 6 years ago

@jouni should we change this to keep the integer number?

We can use height: calc(var(--lumo-size-m) / 9); to get the 4px as it was before. That would evaluate to the following height, depending on the html font size:

font-size: 16px - 4px font-size: 18px - 4.5px font-size: 20px - 5px

Is that fine, or do you have a better idea?

jouni commented 6 years ago

@web-padawan, yeah that seems better. How about if you change the size property directly, like smaller or bigger?

web-padawan commented 6 years ago

Hmm, I didn't take that into account. @platosha do you have any opinion on this? Inventing arbitrary calc() doesn't make me happy, should we just drop the custom CSS property and specify the value in rem explicitly?

jouni commented 6 years ago

Won’t explicit rem value suffer from the exact same issue? For example, if we have the height as 0.25rem (4px by default), it will have fractional pixels when you use any value that is not divisible by 4.

I see two options if we want to avoid fractional sizes:

platosha commented 6 years ago

IMO, the Proper Fix™ should prevent this issue from happening again.

We can use height: calc(var(--lumo-size-m) / 9); to get the 4px as it was before.

This is not a Proper Fix™. It is required then for --lumo-size-m to be a multiple of 9 in order to make the issue not happen. We don’t have any means to ensure that --lumo-size-m follows this rule.

Won’t explicit rem value suffer from the exact same issue?

Yeah, it kind of would. Choosing from depending on the rem size alone vs on --lumo-size-m and the rem size, the first is more reliable. I would say, it’s an acceptable fix.

Use a fixed pixel value

That’s the best fix in durability, but worst in flexibility. But maybe lack of flexibility is acceptable, considering that using theme modules developers can still customize it. Acceptable.

Introduce a new custom property for this use case. I was thinking of a --lumo-divider-size, which could be 1px by default, and we could either use that (×4) or have a few different divider sizes like S (1px), M (2px) and L (4px).

Introducing CSS custom properties for values like 1px and 2px IMO does not bring much sense over using the values directly.

Consider a property that has 1px as default. Suppose you want to use a custom size instead of 1px. It is really hard to do without introducing similar fractional size issues around. You are effectively limited to multiples of 1px. Closest custom value is 2px means doubling the initial size, which might be too much already.

What could be a potentially viable option for Lumo, I think, is introducing a unit size of, let’s say, 8px, and postulating that a unit size has to be an even number of pixels. Then adjusting all the sizes to be derived as a multiple of the unit (or the half-unit when required, e. g., for the progress bar height 😄). That would allow to uniformly scale the derived values by setting the unit size in the parent context. Closest viable options: 6px and 10px, they are 75% and 125% of compared to default unit size, not too far, not too close. That is quite a big change, though, maybe in some future major...

As for now, from the above options, using fixed px value seems the best for me. I am not too worried of inflexibility, because we already have means to customize that, and we could add more later on.

platosha commented 6 years ago

A really specific custom property might also be fine, like --lumo-progress-bar-height

That doesn’t sound bad, but seems unnecessary too at this point, considering above. Defaulting to calc(var(--lumo-size-m) / 9) could potentially produce this issue again. Defaulting to some fixed number does not add much extra value over using that fixed number in the styles IMO.