Paths

Table of Contentst

Differential D7232

[native] Extract LoadableImage component
ClosedPublic
Actions

Authored by bartek on Mar 29 2023, 6:40 AM.

Tags

None

Referenced Files

	F1708283: D7232.diff
	Mon, May 6, 6:55 AM

	Unknown Object (File)
	Sun, May 5, 5:57 AM

	Unknown Object (File)
	Sat, May 4, 2:16 AM

	Unknown Object (File)
	Thu, May 2, 11:20 PM

	Unknown Object (File)
	Sat, Apr 27, 10:29 PM

	Unknown Object (File)
	Sat, Apr 27, 10:29 PM

	Unknown Object (File)
	Sat, Apr 27, 10:29 PM

	Unknown Object (File)
	Sat, Apr 27, 10:29 PM

View All 98 Files

Subscribers

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

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

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

Rebase before landing. Added useMemo for styles.

Harbormaster completed remote builds in B17835: Diff 24428.Mar 30 2023, 10:22 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

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

Path

Size

native/

media/

loadable-image.react.js

69 lines

remote-image.react.js

64 lines

Diff 24328

native/media/loadable-image.react.js

Loading...

native/media/remote-image.react.js

Loading...