use srcset for user pictures #451
Conversation
src/components/ui/UserPicture.vue
Outdated
| return [40, 100, 264] | ||
| .map( | ||
| (s) => | ||
| `${this.avatar.picture}?size=${s} ${(s / this.slot).toPrecision( |
There was a problem hiding this comment.
Since you can pass any number to this.slot, we should probably do some checks before we divide by 0.
There was a problem hiding this comment.
Good point! I'll get that changed
| )}x`, | ||
| ) | ||
| .map((s) => { | ||
| const res = (s / this.slot).toPrecision(2); |
There was a problem hiding this comment.
So, this will still return "NaN" if this.slot is invalid. What should we do when we get NaN?
There was a problem hiding this comment.
Actually looking closer at the code, I don't think we need to check to see if this.slot is 0/invalid/etc. since it's not a prop and we set it in the updateSize method, so it will only ever be 40 or 100 or 264.
There was a problem hiding this comment.
That's a fair argument. What I would say then is that I would actually be very explicit about those being the only values in this component and even put them in their own constants, especially since the constants are used several places in the component.
const SMALL_PICTURE_SIZE = 40;
const MEDIUM_PICTURE_SIZE = 100;
const LARGE_PICTURE_SIZE = 264;
Or you could get fancy with enums. I feel like that would make your argument more evident and understandable for other developers who come after you.
(personal opinion, i'd still do the NaN check; feels like good defensive coding)
There was a problem hiding this comment.
I'd simplify this overall.
Just have something like:
const SRC_SETS = {
"40": [[40, "1x"], [100, "2x"]],
"100": [[100, "1x"], [268, "2x"]],
"264": [[264, "1x"], [528, "2x"]],
}And don't do the math. +1 on using constants for the sizes.
| )}x`, | ||
| ) | ||
| .map((s) => { | ||
| const res = (s / this.slot).toPrecision(2); |
There was a problem hiding this comment.
That's a fair argument. What I would say then is that I would actually be very explicit about those being the only values in this component and even put them in their own constants, especially since the constants are used several places in the component.
const SMALL_PICTURE_SIZE = 40;
const MEDIUM_PICTURE_SIZE = 100;
const LARGE_PICTURE_SIZE = 264;
Or you could get fancy with enums. I feel like that would make your argument more evident and understandable for other developers who come after you.
(personal opinion, i'd still do the NaN check; feels like good defensive coding)
Testing in Firefox using
layout.css.devPixelsPerPxsuggests Firefox prefers to downscale a higher resolution image than upscale a lower resolution one (which is what we want).Running
layout.css.devPixelsPerPxat 2.6x for instance, loads the 264px avatar in the 40px slot even through 100/40px is closer (at 2.5x).528px not yet included as Firefox doesn't discard 404ing images and use a lower resolution instead, so we need to finish/deploy the changes on the backend first.