UI: fix ratings stars filling #254

Francesco Bellini · 2024-09-24T23:49:41+02:00

Francesco Bellini commented

2024-09-24 23:49:41 +02:00

Hey there!

Apparently, different browsers have different ideas about rendering the rating stars of our beautiful extensions (made using svg mask).

In particular, using Firefox, i noticed that stars are kind of cropped (in filling) at the top:

While Chrome, seems to render better:

But still not covering all the svg shape (compare with next, sharper)

And, you know, landing page always has 8 top rated addons, so this stars are always visible when entering from the main door...

As a solution, i incrased the height of the inner <span> (the filling layer) by 2px (while preserving the variable --star-size).

Sharper than ever, everywhere!

Why 2px?

Because firefox is missing 2px, while chrome is missing 1 px.

Since the filling span is masked, also higher values are fine (even extreme one), but i used the minimum necessary to fix Firefox (the browser with the shorter filling).

This seems to render the same across the browsers i tested (firefox, firefox mobile, chrome, safari).

Found and tested on multiple screens (1080p, win/linux) and my phone, using the default 100% zoom.
The problem was found in the addon card and the rating rows.
From my testing, all the stars of the platform benefit from this (and the one of the ratings stats card was fine, and still is).

We all deserve 5-stars rated stars!

Hey there! Apparently, different browsers have different ideas about rendering the rating stars of our beautiful extensions (made using svg mask). In particular, using Firefox, i noticed that stars are kind of cropped (in filling) at the top: ![immagine](/attachments/0318ab7a-bdd8-4950-8764-df7d67c42886) While Chrome, seems to render better: ![immagine](/attachments/6df383d9-5ae1-4a4a-bd06-8100073b90f0) *But still not covering all the svg shape (compare with next, sharper)* And, you know, landing page always has 8 top rated addons, so this stars are always visible when entering from the main door... --- As a solution, i incrased the height of the inner `<span>` (the filling layer) by 2px (while preserving the variable `--star-size`). ![immagine](/attachments/7ebdfce4-020c-4668-aba1-ab3e7cfd247a) *Sharper than ever, everywhere!* **Why 2px?** Because firefox is missing 2px, while chrome is missing 1 px. *Since the filling span is masked, also higher values are fine (even extreme one), but i used the minimum necessary to fix Firefox (the browser with the shorter filling).* This seems to render the same across the browsers i tested (firefox, firefox mobile, chrome, safari). > Found and tested on multiple screens (1080p, win/linux) and my phone, using the default 100% zoom. > The problem was found in the addon card and the rating rows. > From my testing, all the stars of the platform benefit from this (and the one of the ratings stats card was fine, and still is). --- We all deserve 5-stars rated stars!

immagine.png

524 B

immagine.png

602 B

immagine.png

595 B

Francesco Bellini added 1 commit 2024-09-24 23:49:43 +02:00

ui: fix ratings stars filling 6ec1f4801b

Francesco Bellini force-pushed fix-ratings-stars-fill from 6ec1f4801b to 4005eff179

2024-09-29 13:00:59 +02:00

Compare

Márton Lente was assigned by Pablo Vazquez

2024-10-01 15:47:10 +02:00

Márton Lente merged commit 7906024359 into main

2024-10-03 16:49:09 +02:00

Márton Lente referenced this issue from a commit

2024-10-03 16:49:11 +02:00

Fix: Cropped ratings stars filling

Márton Lente commented

2024-10-03 16:56:28 +02:00

@Francesco-Bellini thanks for the nice fix, very useful again! I tested in multiple browsers and works reliably. 👍

Just a minor notice for later: When working with explicit sizes, we usually use rems instead of pxs, that are matched to pixels' scaling on the root element level. See this table for details. (The pull request was perfect as is, converting px values to rems is just an ongoing initiative I thought was worth mentioning for later.)

@Francesco-Bellini thanks for the nice fix, very useful again! I tested in multiple browsers and works reliably. 👍 Just a minor notice for later: When working with explicit sizes, we usually use `rem`s instead of `px`s, that are matched to pixels' scaling on the root element level. See [this table](https://projects.blender.org/infrastructure/web-assets/wiki/v2-guidelines-and-notes.md#units) for details. (The pull request was perfect as is, converting `px` values to `rem`s is just an ongoing initiative I thought was worth mentioning for later.)

Francesco Bellini commented

2024-10-03 18:06:02 +02:00

Hi @martonlente!
Glad it was a quick PR for you to handle!

I'm sorry for the pixels. I wasn't aware of this fixed relationship between them. Thanks for pointing the resources!
I looked around and understood why and how this is inforced. Nice!

I'm taking some time to better understand the mind-set. Hope i'm making progess.

P.s. Just a heads-up: I'm almost done working on a new PR. No fixes this time. I'm trying to bring something new :) We'll see.

I know it's kind of unreleated here, but...

I was investigating on this relationship between pixels and rem, and, as your resource states, it's done by setting the html font-size to 62.5% (which directly affects rem).
At the same time, this percent applies to the default browser font-size, which is usually 16px, but that can be changed from browser preferences, setting the global font size to another value.

I never changed the font-size for browsers personally (but i'm guessing someone is doing so), but i noticed that changing this global setting from the browser, along with changing the font-size of text, as obvious, also mess with the spacing of paddings, margins, borders that rely on rems.

Since we are using font-related units in non-text related elements, users changing that preference, can lead to a bad layout (regardless text elements).

Setting browser font size to 24px (or 30px, i don't remember...)

Maybe it's a known thing, and we are fine with it...
I just wanted to bring it to your attention as i just found it out, while trying to better understand how it works...

Hi @martonlente! Glad it was a quick PR for you to handle! I'm sorry for the pixels. I wasn't aware of this fixed relationship between them. Thanks for pointing the resources! I looked around and understood why and how this is inforced. Nice! I'm taking some time to better understand the mind-set. Hope i'm making progess. P.s. Just a heads-up: I'm almost done working on a new PR. No fixes this time. I'm trying to bring something new :) We'll see. --- **I know it's kind of unreleated here, but...** I was investigating on this relationship between pixels and rem, and, as your resource states, it's done by setting the html font-size to `62.5%` (which directly affects `rem`). At the same time, this percent applies to the default browser font-size, which is usually 16px, but that can be changed from browser preferences, setting the global font size to another value. I never changed the font-size for browsers personally (but i'm guessing someone is doing so), but i noticed that changing this global setting from the browser, along with changing the font-size of text, as obvious, also mess with the spacing of paddings, margins, borders that rely on `rem`s. Since we are using font-related units in non-text related elements, users changing that preference, can lead to a bad layout (regardless text elements). _Setting browser font size to 24px (or 30px, i don't remember...)_ ![immagine](/attachments/ab96fae6-a210-4aac-85f7-aefd7da5b8be) Maybe it's a known thing, and we are fine with it... I just wanted to bring it to your attention as i just found it out, while trying to better understand how it works...

immagine.png

150 KiB

👍 1

Márton Lente commented

2024-10-04 11:05:26 +02:00

Hi @Francesco-Bellini,

Your question indeed makes sense, and I don't think there's a right or wrong answer here. As in our design system the body font-size (font height) is the base unit of all scaling, using this 'typographic' unit when defining all explicit sizes makes a lot of sense from both practical and visual perspectives, as this is what makes sizing truly consistent and responsive. I see it as a benefit if all spacings change when adjusting font-sizings in the browser, as they work as a system of sizes. Consistent scaling also has benefits when dealing with operating system level font-scaling issues (e.g. almost all small to medium size Windows laptops ship with font-scaling set to either 125% or 150% on the OS level by default, that needs extra attention in web design).

The only exception I see is media queries, where sizes must be in pixels, as browsers read media queries on the browser and not the root element level. Another good approach some developers do is to use the above rem sizings for vertical spacings (as all vertical sizing is basically work with body text baselines like in book design), and px sizings for hozizontal spacings that doesn't necessarily use the same grid and sizing units.

I hope this gives some ideas why we do the above in Web Assets.

Looking forward to new improvements!

Hi @Francesco-Bellini, Your question indeed makes sense, and I don't think there's a right or wrong answer here. As in our design system the body font-size (font height) is the base unit of all scaling, using this 'typographic' unit when defining all explicit sizes makes a lot of sense from both practical and visual perspectives, as this is what makes sizing truly consistent and responsive. I see it as a benefit if all spacings change when adjusting font-sizings in the browser, as they work as a system of sizes. Consistent scaling also has benefits when dealing with operating system level font-scaling issues (e.g. almost all small to medium size Windows laptops ship with font-scaling set to either 125% or 150% on the OS level by default, that needs extra attention in web design). The only exception I see is media queries, where sizes must be in pixels, as browsers read media queries on the browser and not the root element level. Another good approach some developers do is to use the above rem sizings for vertical spacings (as all vertical sizing is basically work with body text baselines like in book design), and px sizings for hozizontal spacings that doesn't necessarily use the same grid and sizing units. I hope this gives some ideas why we do the above in Web Assets. Looking forward to new improvements!

👍 1

Francesco Bellini deleted branch fix-ratings-stars-fill

2024-10-04 15:56:56 +02:00

Sign in to join this conversation.

No reviewers