Page MenuHomePhabricator
Differential D7232

[native] Extract LoadableImage component
ClosedPublic
Actions

Authored by bartek on Mar 29 2023, 6:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 7:12 PM
Unknown Object (File)
Mon, Nov 4, 3:30 AM
Unknown Object (File)
Mon, Nov 4, 3:30 AM
Unknown Object (File)
Mon, Nov 4, 3:30 AM
Unknown Object (File)
Mon, Nov 4, 3:29 AM
Unknown Object (File)
Mon, Nov 4, 3:29 AM
Unknown Object (File)
Mon, Nov 4, 3:29 AM
Unknown Object (File)
Sun, Oct 27, 1:42 AM
View All Files
Subscribers
tomek

Details

Reviewers
ashoat
atul
ginsu
Commits
rCOMM543f5b955c02: [native] Extract LoadableImage component
Summary

Addressing feedback from https://phab.comm.dev/D7225 - extracted the common part from <RemoteImage> to a new component: <LoadableImage>

Test Plan

Media images are still loaded and displayed correctly.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek created this revision.Mar 29 2023, 6:40 AM
bartek held this revision as a draft.
Herald added a subscriber: tomek. · View Herald TranscriptMar 29 2023, 6:40 AM
bartek mentioned this in D7225: [native] Add component to display encrypted images.Mar 29 2023, 6:42 AM
bartek added a child revision: D7225: [native] Add component to display encrypted images.Mar 29 2023, 6:43 AM
Harbormaster completed remote builds in B17777: Diff 24328.Mar 29 2023, 6:55 AM
bartek published this revision for review.Mar 29 2023, 7:12 AM
ashoat added a comment.Mar 29 2023, 11:45 AM

Just a question. Will leave it on others' queue in case they have review comments

native/media/remote-image.react.js
33–41

Given that both RemoteImage and RemoteImage seem to need this functionality, I'm wondering why you didn't factor it out into LoadableImage

atul accepted this revision.Mar 29 2023, 12:01 PM
atul added inline comments.
native/media/loadable-image.react.js
36

Can we use React.useMemo here now that we're in a functional component?

This revision is now accepted and ready to land.Mar 29 2023, 12:01 PM
bartek added inline comments.Mar 30 2023, 4:42 AM
native/media/loadable-image.react.js
36

Sure, I'll do it

native/media/remote-image.react.js
33–41

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.

bartek mentioned this in D7187: [native] extend avatar component to handle images.Mar 30 2023, 5:11 AM
ashoat added inline comments.Mar 30 2023, 6:09 AM
native/media/remote-image.react.js
33–41

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

ashoat accepted this revision.Mar 30 2023, 6:09 AM
bartek updated this revision to Diff 24428.Mar 30 2023, 10:06 AM

Rebase before landing. Added useMemo for styles.

Harbormaster completed remote builds in B17835: Diff 24428.Mar 30 2023, 10:22 AM
bartek updated this revision to Diff 24431.Mar 30 2023, 11:04 AM

Removed unnecessary state (noticed that sometimes re-renders caused flickering)

Harbormaster completed remote builds in B17836: Diff 24431.Mar 30 2023, 11:20 AM
bartek updated this revision to Diff 24434.Mar 30 2023, 11:30 AM

Rebase

Harbormaster completed remote builds in B17839: Diff 24434.Mar 30 2023, 11:47 AM
Closed by commit rCOMM543f5b955c02: [native] Extract LoadableImage component (authored by bartek). · Explain WhyMar 30 2023, 11:09 PM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMM543f5b955c02: [native] Extract LoadableImage component.
ginsu added a child revision: D7274: [web] introduce ThreadAvatar component.Apr 2 2023, 11:36 PM
ginsu removed a child revision: D7274: [web] introduce ThreadAvatar component.Apr 3 2023, 1:23 AM

Revision Contents
Changeset List

PathSize
native/
media/
69 lines
64 lines

Diff 24328

View Options

native/media/loadable-image.react.js

Loading...
View Options

native/media/remote-image.react.js

Loading...