Addressing feedback from https://phab.comm.dev/D7225 - extracted the common part from <RemoteImage> to a new component: <LoadableImage>
Details
Details
- Reviewers
ashoat atul ginsu - Commits
- rCOMM543f5b955c02: [native] Extract LoadableImage component
Media images are still loaded and displayed correctly.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Just a question. Will leave it on others' queue in case they have review comments
| native/media/remote-image.react.js | ||
|---|---|---|
| 33–41 ↗ | (On Diff #24328) | Given that both RemoteImage and RemoteImage seem to need this functionality, I'm wondering why you didn't factor it out into LoadableImage |
| native/media/loadable-image.react.js | ||
|---|---|---|
| 36 ↗ | (On Diff #24328) | Can we use React.useMemo here now that we're in a functional component? |
| native/media/loadable-image.react.js | ||
|---|---|---|
| 36 ↗ | (On Diff #24328) | Sure, I'll do it |
| native/media/remote-image.react.js | ||
| 33–41 ↗ | (On Diff #24328) | I tried that at first, but it turned out to complicate code more. EncryptedImage needs the reconnection logic also in the hook, so we'd end up with duplicated connection-status logic anyway. |
| native/media/remote-image.react.js | ||
|---|---|---|
| 33–41 ↗ | (On Diff #24328) | Ah, I see... LoadableImage can't handle attempt because the component that renders it, EncryptedImage, also needs awareness of attempt so that decryptMedia gets called again. That makes sense |