https://linear.app/comm/issue/ENG-4271/encrypted-media-load-failures-display-spinners-infinitely-on-native. In addition, upload error
icon has been changed, to match new error icon (and icon available on web).
Screenshots:
Details
Details
- Reviewers
bartek tomek kamil inka - Commits
- rCOMMfdf85ed0dc23: [native] Fix infinite image loader
- Change url to local blob service url in lib/facts/blob-service.js
- Change useBlobServiceUploads to true in native/input/input-state-container.react.js
- Run blob service
- To emulate error, disconnect blob service or change result.success to false in native/media/encrypted-image.react.js
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- patryk/encrypted-image
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Can you attach a screenshot to the diff description of how the error icon looks like?
| native/media/loadable-image.react.js | ||
|---|---|---|
| 70 | We should avoid using color constants. Instead, we should use something from our design system (something from Colors type from native/theme/colors.js). Also, it is a good idea to check how it looks like in both light and dark themes. | |
| native/media/encrypted-image.react.js | ||
|---|---|---|
| 82–83 ↗ | (On Diff #28374) | We discussed this offline - I'm not too fond of this hack because it makes the code flow unobvious. I prefer adding a new, optional prop isError (or forceError, I don't have a good idea for the name) to <LoadableImage> which can work together with the error state you introduced. This way we can explicitly tell why the error indicator is displayed. |
| native/media/loadable-image.react.js | ||
| 54–69 ↗ | (On Diff #28374) | I don't know if this will be more readable, just wondering about inverting the code flow here. Let's suppose we have loaded = false; error = true. The statusIndicator is first created for the loading state and then overwritten by the error indicator. Using else-if we can avoid this. |
| native/media/loadable-image.react.js | ||
|---|---|---|
| 54–69 ↗ | (On Diff #28374) | I wanted to use a guard clause because I find it more readable. However, as you mentioned, statusIndicator is updated twice. It's not a big deal, I think, but we can avoid unnecessary variable update. |
| native/chat/inline-multimedia.react.js | ||
|---|---|---|
| 144–147 ↗ | (On Diff #28394) | Looking at the attached screenshots, you tested how this styling change affects the existing usage of uploadError in inline-multimedia.react.js. |