Show image description if present #2103
Conversation
Add a button to expand the image description if applicable
|
Oh dang, the auto-deploy to preview is really slick! Many props to whoever got that set up! One thing I remembered after checking out the preview: an awkward bit (that would be alleviated by having the text itself be the button) is that a button still shows up even if the text is only one line long, and clicking on it effectively does nothing. I don't know if there's a great way around this - can we determine at render time if the textbox is overflowing somehow? And even if we could, we'd have to re-check if the user zoomed or resized or what have you...seems maybe more complicated than it's worth. |
There was a problem hiding this comment.
Thanks for putting together this PR. I had some minor nitpicks.
Aside from that, I think the general idea is good – there's room on the left for the ℹ️ icon thanks to the pinch-zoom icon, so there's a pleasant symmetry by adding it. I also think it's a good idea to hide it when the description is missing.
As for the placement of the text itself, I would somewhat prefer for larger text center-positioned on top of the entire image. (Or maybe positioned over the bottom third, kind of like a film subtitle.) The current position seems a bit off to me.
Also I'm not sure if there's something wrong with how I'm using it, but it seems like the info toggle button doesn't actually work. The description seems always visible, whether the button is toggled or not.
Thanks again for your work on this!
| <IconButton | ||
| className="media-control-button" | ||
| svgClassName="media-control-button-svg" | ||
| pressable={hasDescription} |
There was a problem hiding this comment.
This pressable={hasDescription} is unnecessary; we're already in an if hasDescription block.
| pressable={hasDescription} | |
| pressable={true} |
There was a problem hiding this comment.
Oh! Yeah good catch - this is left over from an earlier iteration that didn't have the if blocks here.
| <IconButton | ||
| className="media-control-button" | ||
| svgClassName="media-control-button-svg" | ||
| pressable={true} | ||
| pressable={canPinchZoom} |
There was a problem hiding this comment.
Same here, this can just be pressable={true}.
| pressable={canPinchZoom} | |
| pressable={true} |
| pressed={pinchZoomMode} | ||
| label="{intl.enterPinchZoom}" | ||
| pressedLabel="{intl.exitPinchZoom}" | ||
| href="#fa-search" | ||
| on:click="togglePinchZoomMode()" | ||
| ariaHidden={!canPinchZoom} |
There was a problem hiding this comment.
| ariaHidden={!canPinchZoom} | |
| ariaHidden={false} |
There was a problem hiding this comment.
I can just leave ariaHidden off completely here, yeah?
| }), | ||
| computed: { | ||
| length: ({ mediaItems }) => mediaItems.length, | ||
| dots: ({ length }) => times(length, i => ({ i })), | ||
| canPinchZoom: ({ mediaItems }) => !mediaItems.some(media => ['video', 'audio'].includes(media.type)), | ||
| hasDescription: ({ mediaItem }) => mediaItem.description, |
There was a problem hiding this comment.
A bit more clear that this is a boolean:
| hasDescription: ({ mediaItem }) => mediaItem.description, | |
| hasDescription: ({ mediaItem }) => !!mediaItem.description, |
There was a problem hiding this comment.
Good call - I've come around to slightly prefer Boolean() in these situations, but I think either is fine if you have a style preference.
Implements #2102. This is somewhat a proof-of-concept, it's a pretty basic implementation but it does the job!
I'm not actually sure it's worth using the lefthand button slot, it may make more sense to just have tapping on the description expand it instead - we could add something to indicate it was tappable? I'm not sure what that would be (Tusky has a little line in the top center and an outline indicating a "drawer" of sorts, for instance)
This also shows the first line of the image description by default, which takes up a little more vertical space, but I think it's worthwhile - making image descriptions more visible in general is a net good, I think. The button showing up also would indicate this, but for short descriptions the user wouldn't have to do anything.
I also considered making a three-state button, where it would start as hidden, and tapping would toggle through hidden->first line->full, but that seems not really an improvement over just hidden->full.
I made the max height of the expanded box shove the image out of the way and fill up to half the screen, and scroll after that. Another option would be to have it expand into an overlay over the image and take up as much height as it needs - if someone is reading the description, they're generally fine not looking at the image for a bit - but that was harder to implement, so I went with the easy way for the proof-of-concept.