Details
- Reviewers
bartek ashoat tomek inka - Commits
- rCOMMf447ed220729: [web] Fix stretched images bug
Run web app and check if images are stretched.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- patryk/image-height
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Generally I'd go the other way around:
Firstly, fix the stretching issue AND avoid that jump when it finishes loading. This is the crucial issue here
Then figure out how to deal with the too large spinner
web/media/multimedia.react.js | ||
---|---|---|
163 |
|
Wouldn't commenting this code out result in the thumbhash having the wrong dimensions? Doesn't the thumbhash need the exact dimensions passed to it?
Fix thumbhash wrong dimensions and hide border when image tag doesn't have src attribute.
web/media/encrypted-multimedia.react.js | ||
---|---|---|
24 ↗ | (On Diff #28643) | There is no aspect-ratio attribute in CSSStyleDeclaration. |
web/media/loadable-video.react.js | ||
23 ↗ | (On Diff #28643) | There is no aspect-ratio attribute in CSSStyleDeclaration. |
I tested this locally and it solves the issue! Great work @pklatka. Going to pass back to you with just some questions / minor suggestions inline
web/media/encrypted-multimedia.react.js | ||
---|---|---|
24 ↗ | (On Diff #28643) | This type might be useful in other contexts. For native we have native/types/styles.js – can you create web/types/styles.js, and define this as a new type there (eg. export type CSSStyle or something)? |
web/media/media.css | ||
23 ↗ | (On Diff #28643) | Can you explain this change and show a before / after screenshot? |
26–28 ↗ | (On Diff #28643) | Can you explain this change and show a before / after screenshot? I'm also curious why we need not([src]) – maybe include a screenshot of what this would look like if we skipped that part |
web/media/media.css | ||
---|---|---|
26–28 ↗ | (On Diff #28643) | I guess the not(src) refers to the thumbhash image where we haven't yet provided the actual decrypted src. But good point - this is so unobvious, it's worth explaining and maybe adding a comment here |
I need a lot more context. Please try to explain in detail the decisions behind each of these
web/media/media.css | ||
---|---|---|
23 ↗ | (On Diff #28643) | Where did you get this border-radius value? I tried to git grep in the codebase, but I wasn't able to find it: # cd web/chat # git grep border-radius chat-input-bar.css: border-radius: 8px; chat-thread-list-item-menu.css: border-radius: 4px; chat-thread-list.css: border-radius: 1.68px; chat-thread-list.css: border-radius: 15px; chat-thread-list.css: border-radius: 5px; edit-text-message.css: border-radius: 8px; edit-text-message.css: border-radius: 4px; inline-engagement.css: border-radius: 16px; inline-engagement.css: border-radius: 16px 0 0 16px; inline-engagement.css: border-radius: 0 16px 16px 0; message-tooltip.css: border-radius: 8px; message-tooltip.css: border-radius: 8px; typeahead-tooltip.css: border-radius: 8px; typeahead-tooltip.css: border-radius: 4px; I'm also concerned that this CSS selector may inadvertently affect places we don't intend for it to affect. Can you identify all of the locations affected by this selector, and see if this border-radius is appropriate for all of them? For instance, does the span.multimedia > .multimediaImage > img, span.multimedia > .multimediaImage > video selector affect the ChatInputBar or the MultimediaModal? High-level feedback: it shouldn't be necessary for a reviewer to investigate things like this... as the diff author, you should proactively identify places that might be confusing, and explain with inline comments where things are coming from / how decisions were made |
26–28 ↗ | (On Diff #28643) | Weird. Is this really a border? If so, shouldn't we hide it by disabling the border, rather than disabling the visibility of the whole img? If we can disable the border, then I suspect the not([src]) part might not be necessary (since we don't want to show a border in any condition) |
web/media/media.css | ||
---|---|---|
23 ↗ | (On Diff #28643) | From here. It looks like it is being set on message wrapper. No wonder that grep didn't catch that (I discovered this value when I was playing with CSS in browser). And yes, it affects ChatInputBar uploads, my bad, could have mentioned this. Probably that's why I used inline border-radius rather than using it in media.css. The main motivation to use inline border-radius was, as I wrote before, to round long images which render with small height (less than 50px). Because we set min-height: 50px here, images, as well as videos, with rendered height < 50px could not be rendered with borders. I suggest to return to inline declaration of border-radius. |
26–28 ↗ | (On Diff #28643) | If border: 0/none or outline: 0/none worked, I would use it. It's the default "special" border that appears when you use an img element with an a src attribute set to something that doesn't exist (or you don't use src at all). We could also set some blank image as source to hide this border, but it would be the same as hiding this tag with CSS. You can read more here. |
web/media/media.css | ||
---|---|---|
23 ↗ | (On Diff #28643) |
|
26–28 ↗ | (On Diff #28643) | Thanks for explaining!! This is weird, but your links helped me understand why it's necessary |
web/media/media.css | ||
---|---|---|
23 ↗ | (On Diff #28643) | Let's say we encounter this situation. User uploads "normal-sized" images and decides to upload a very long image. As you can see, this normal image covers completely our message container (temporary red color is not visible). However long images leave some free space, due to min-height: 50px. And because of that, our long image is not rounded, as image with normal size. So I've decided to add border-radius to avoid situation visible in the screenshot above. But what I found out today, is that we don't set this radius for every corner every time. We could inherit border settings from message container but we will end up getting something like this: Without red background: Because of min-height, there is quite a large gap between messages which I don't think matches our design. To solve this problem we have three options:
I feel like this issue fix should be a separate diff. Below I've attached some visualisations how second and third option could look like: Third option: |
Thanks for the helpful visuals! That made it very clear what the problem is.
I agree that this should be handled in a separate diff. Can you create a separate Linear task for it, and update this diff to get rid of the border-radius changes?
I think my preferred solution would be if we could "zoom in" to the long image, and "clip" the left and right sides. So then it would appear with the min-height of 50px, and would still have the correct aspect ratio. Is something like this possible with eg. object-fit CSS?
It might be good to update these three existing sites where we use CSSStyleDeclaration to now use CSSStyle:
web/media/encrypted-multimedia.react.js: +elementStyle?: ?Shape<CSSStyleDeclaration>, web/media/loadable-video.react.js: +elementStyle?: ?Shape<CSSStyleDeclaration>, web/utils/typeahead-utils.js: const textareaStyle: CSSStyleDeclaration = window.getComputedStyle(