[native] Fix infinite image loader
ClosedPublic
Actions

Authored by • patryk on Jul 3 2023, 6:39 AM.

Details

Reviewers

Commits

rCOMMfdf85ed0dc23: [native] Fix infinite image loader

Summary

Screenshots:

Test Plan

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

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

• patryk requested review of this revision.Jul 3 2023, 6:56 AM

Can you attach a screenshot to the diff description of how the error icon looks like?

native/media/loadable-image.react.js
70 ↗	(On Diff #28349)	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.

Use colors declared in native/themes/color.js.

bartek requested changes to this revision.Jul 4 2023, 5:47 AM

bartek added inline comments.

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. What do you think?

This revision now requires changes to proceed.Jul 4 2023, 5:47 AM

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.

Add errorOccured prop and change guard clause to else-if.

bartek added inline comments.

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`.