Page MenuHomePhabricator

[native] Add thumbHash prop to image components
ClosedPublic

Authored by bartek on May 9 2023, 8:08 AM.
Tags
None
Referenced Files
F1439912: D7765.id26598.diff
Thu, Mar 28, 9:02 PM
F1439911: D7765.id26582.diff
Thu, Mar 28, 9:02 PM
F1439910: D7765.id26501.diff
Thu, Mar 28, 9:02 PM
F1439908: D7765.id26312.diff
Thu, Mar 28, 9:02 PM
F1439886: D7765.id.diff
Thu, Mar 28, 9:01 PM
F1439862: D7765.diff
Thu, Mar 28, 8:53 PM
Unknown Object (File)
Sun, Mar 3, 4:23 AM
Unknown Object (File)
Sun, Mar 3, 4:23 AM
Subscribers

Details

Summary

This diff exposes the expo-image's placeholder prop (docs). For <LoadableImage> and <RemoteImage> it is passed directly for flexibility.
For encrypted images, the thumbHash is encrypted so it must be decrypted before creating a placeholder. For non-encrypted images, it can be passed directly.

The part where multimedia.react.js receives the thumbhash prop will be added in next diffs whenkeyserver support is added.

Depends on D7761

Test Plan

Here are a few thumbhash examples that you can pass to the prop:

// these are raw, non-encrypted thumbhash strings
const thumbhashes = [
  '1EgOHYQImHiZZ4iCe3eWeAinolA8',
  '5xcSHQZpeI94KIenaHhoh2ufo/Y4',
  'EQgKHQZlh2122Hb4dm2Gh2hwkQc3',
  'GTSBBIAmOIlphnApR+P7ijmPhYUIR4h3Bg',
  'JRkaPQh4d394V4doeHd3h3iAgAcI',
  'TwgOFQKniXV1+JeYh3u2mLZ1b1v3',
  'YGkKHQKnh392CGdXpXVoeptu0IgF',
  'E9cJVRB6h493R3iIeHmXZ3hwhAg3',
];
const thumbhash = thumbhashes[Math.random() % thumbhashes.length];
const placeholder = { thumbhash };
`

Simulator Screen Shot - iPhone 14 Pro - 2023-05-09 at 17.31.58.png (2×1 px, 399 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.May 9 2023, 8:39 AM
atul requested changes to this revision.May 15 2023, 11:11 AM

Looks good at a high level. Thanks for including examples in the Test Plan!

Requesting changes for response on thumbhash vs thumbHash naming convention, think we should be more clear there

native/media/encrypted-image.react.js
47 ↗(On Diff #26312)

Personally, I think it would be cleaner for decrypted data to "flow down through" components instead of handling decryption within React components where data is displayed.

Feel like decryption should be a "step" (like rehydrating redux?) that's done "up front" and "React" can just handle data that's been processed and "ready to render." Understand that might not be practical given the way the app is currently though

(Nothing actionable here, just a thought)

47 ↗(On Diff #26312)

Feel like the distinction between thumbhash and thumbHash isn't clear enough (capitalization of "H").

Could we add some prefixes (eg encryptedThumbHash/decryptedThumbHash) to clarify things?

This revision now requires changes to proceed.May 15 2023, 11:11 AM
native/media/encrypted-image.react.js
47 ↗(On Diff #26312)

Yes, good point

Made variable names more clear

Sweet, thanks for addressing feedback!

This revision is now accepted and ready to land.May 16 2023, 9:36 AM