fix(web): remove black bezels + better integrate ActivityStatus #10667
Conversation
pyorot
changed the title
|
An extra comment – I wasn’t sure how to handle removing the text so left the checks, unused variables etc for review. My thoughts were that it made the most sense now to do it, also because no other controls have text next to them, but that it could debatably be useful in future. |
| <div class="text-lg">{$t('say_something')}</div> | ||
| <!-- the say_something text is removed for being obtrusive but could be put back in --> | ||
| <!-- if it disappears on narrowing the viewport or once user control auto fadeout is implemented --> | ||
| <!-- {:else if !isShowActivity} <div class="text-lg">{$t('say_something')}</div> --> |
There was a problem hiding this comment.
How about only hiding it on mobile?
There was a problem hiding this comment.
Maybe the way forward is lots of screenshots. Fortunately, the usual-looking video player is back today on my setup lmao. So we can have some examples courtesy of dread dog...
There was a problem hiding this comment.
First idea: disable the text whenever the info pane is showing (existing behaviour: only if activity pane is showing.
(I will always be pro–always disable personally because no other user buttons in the asset viewer have text).
immich (release):
immich (pr branch):
google photos:
There was a problem hiding this comment.
Second idea: <600px viewport width (examples are 550×400px)
immich (release):
immich (pr branch):
google photos:
google photos' logic is basically:
- side pane showing → activity status: no text + bottom row
- side pane hidden + <600px width → activity status: no text + third row ish (only for videos)
- side pane hidden + >600px width → activity status: yes text + bottom row
There was a problem hiding this comment.
On google photos, this overlap is annoying, but let's say they fixed it by raising the activity status to third row as they do on narrow width viewport. Then the text is still unobtrusive, but only because it fades out after a few seconds, which immich doesn't currently do (is probably fairly important to work on for stable release?).
| @@ -682,7 +682,7 @@ | |||
| /> | |||
| {/if} | |||
| {#if $slideshowState === SlideshowState.None && isShared && ((album && album.isActivityEnabled) || numberOfComments > 0)} | |||
| <div class="z-[9999] absolute bottom-0 right-0 mb-4 mr-6"> | |||
| <div class="z-[9999] absolute bottom-0 right-0 mb-12 mr-4 justify-self-end"> | |||
There was a problem hiding this comment.
I don't think justify-self-end works here. What browser is your screenshot from? Getting the bottom spacing right is pretty difficult, because there's a lot of variability between browsers and even within the same browser when zooming.
There was a problem hiding this comment.
The browser is windows chrome, same as today's screenshots (the video controls yesterday changed owing to videos consistently failing to load). justify-self-end is imported from reverting the change that introduced the black bezels (#8757), but I can look further into it.
There was a problem hiding this comment.
I am not noticing a difference from removing justify-self-end using dev tools so it was possibly a trivial fix in the aforementioned change.
There was a problem hiding this comment.
Zooming seems to keep things consistent on windows chrome, though things overlap more in all cases since the usual video control style came back to my setup, so maybe I would advise switching to row 2.5 rather than 2 from the bottom (where row 1 is by definition symmetric to the top row). Even like this, it is usable though.
There was a problem hiding this comment.
Windows firefox looks superb on pr branch at all zoom levels (within reason haha).
My iPhone won't connect to the Vite server unfort.
|
r.e. mr-4 → mr-3: this is mr-3. Obviously these are all approximations since the styling is implemented differently to the top control bar.
|
|
With this PR, I'm personally happy to merge any change that gets rid of the black bottom margin, even if it's a straight reversion to where the controls overlap again. So I'm happy to take any design cues for where to put the controls and what text to show when. |
My entire description was deleted so I'll keep it brief I guess.
Fixes #10526
Couldn't test properly cos the headers-already-sent bug broke the video and changed the appearance of the controls but think it's OK.
Tried to right-align the buttons and make ActivityControl as-if a kind of second-row from the bottom, symmetrical to the top row. TODO: It should be first-row from bottom on PhotoViewer but idk how to condition in Svelte macros for PhotoViewer vs VideoViewer appearing.
Tried to also make ActivityControl less obtrusive, so removed the "Say Something" text (because it does not fade out nor disappear if the viewport width is decreased). I left the code in a comment and noted these two conditions for maybe bringing it back, tho it's a bit subjective whether it's useful or not.