refactor: thumbnail rendering with Image component, focus styling, data-selected#26305
refactor: thumbnail rendering with Image component, focus styling, data-selected#26305
Conversation
midzelis
added
changelog:feature
changelog:enhancement
and removed
changelog:feature
labels
midzelis
force-pushed
the
push-utotpurmrqyo
branch
2 times, most recently
from
445aa48 to
997fb79
Compare
| {style} | ||
| alt={loaded || errored ? altText : ''} | ||
| {title} | ||
| class={['object-cover', optionalClasses, imageClass]} |
There was a problem hiding this comment.
Why is optionalClasses removed?
| document.removeEventListener('pointermove', moveHandler, true); | ||
| }; | ||
| }); | ||
| const backgroundColorClass = $derived.by(() => { |
There was a problem hiding this comment.
The gray background for selected thumbnails is now gone, is this an intentional change?
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Unintentional - nice catch. The reason why the background is being made transparent is so that the 'AlphaBackground' for transparent images can show through. Though, it was originally an actual overlay that used z-order and a wrapping div to contain it. I didn't like the extra DOM element - so I'm just setting a solid background-color on the itself, which will draw behind the image - so I think I'll be able to revert the transparent. If not, I can make it so the background only draws when its selected. (As a backup)
There was a problem hiding this comment.
I'm reverting the border changes, and the unintentional background change. Transparent images still have a gray (slightly different tone) behind them.
| <!-- Outline on focus --> | ||
| <div | ||
| class={[ | ||
| 'pointer-events-none absolute z-1 size-full outline-immich-primary dark:outline-immich-dark-primary group-focus-visible:rounded-lg group-focus-visible:outline-2 group-focus-visible:-outline-offset-2', |
There was a problem hiding this comment.
I’m not a fan of the updated focus outline. It’s thinner so less visible, and it always uses rounded corners even though the thumbnails themselves don’t.
There was a problem hiding this comment.
I don't have time to address all your comments right now, but this change also updates the thumbnails to use rounded corners when its outlined:
There was a problem hiding this comment.
In general, I feel like all selections (outlines) need to be consistent in the app. Personally, I thought the 4px in blue was pretty ugly and didn't match the rest of the theme within the app. But, if the vote is for 4px outlines - then we should standardize on 4px focus outlines for every component on the page - including the textboxes, buttons, menus, etc.
| onStart?.(); | ||
| }); | ||
| return () => { | ||
| destroyed = true; |
There was a problem hiding this comment.
destroyed is never set to false again, so onLoad and onError will never fire after the src is changed
There was a problem hiding this comment.
Correct, working as designed.
Images should only be set once. If you want to re-load an <Image> put it in a {#key}
| let optionalClasses = $derived([ | ||
| [ | ||
| curve && 'rounded-xl', | ||
| circle && 'rounded-full', | ||
| shadow && 'shadow-lg', | ||
| (circle || !heightStyle) && 'aspect-square', | ||
| border && 'border-3 border-immich-dark-primary/80 hover:border-immich-primary', | ||
| brokenAssetClass, | ||
| ] | ||
| .filter(Boolean) | ||
| .join(' '), | ||
| brokenAssetClass, | ||
| ]); |
There was a problem hiding this comment.
| let optionalClasses = $derived([ | |
| [ | |
| curve && 'rounded-xl', | |
| circle && 'rounded-full', | |
| shadow && 'shadow-lg', | |
| (circle || !heightStyle) && 'aspect-square', | |
| border && 'border-3 border-immich-dark-primary/80 hover:border-immich-primary', | |
| brokenAssetClass, | |
| ] | |
| .filter(Boolean) | |
| .join(' '), | |
| brokenAssetClass, | |
| ]); | |
| let optionalClasses = $derived([ | |
| curve && 'rounded-xl', | |
| circle && 'rounded-full', | |
| shadow && 'shadow-lg', | |
| (circle || !heightStyle) && 'aspect-square', | |
| border && 'border-3 border-immich-dark-primary/80 hover:border-immich-primary', | |
| brokenAssetClass, | |
| ]); |
|
It looks like you are changing the design and look while also refactoring. Please only do one or the other. |
midzelis
force-pushed
the
push-utotpurmrqyo
branch
from
997fb79 to
aa24b8a
Compare
3 participants
No description provided.