Looks good to me! I would love it if someone more experienced in Rust would take a look, @bartek can you take a final round?

Retry

In D9354#275219, @kamil wrote:

LGTM, only one nit: you can probably replace Array<> with $ReadOnlyArray.

LGTM, only one nit: you can probably replace Array<> with $ReadOnlyArray.

Rebase before landing

Rebase before landing

1.Rebase before landing

2. Use NEXT_CODE_VERSION to prepare for the release

Rebase begore landing

rebase before landing

In D9366#275107, @atul wrote:

Double Extra Large??

rebase before landing

rebase before landing

rebase before landing

rebase before landing

address atul's feedback

rebase before landing

rebase before landing

rebase before landing

https://linear.app/comm/issue/ENG-5166/improve-refactoring-of-fullscreenviewmodal-compoment

In D9362#275121, @atul wrote:

Makes sense, wonder if there's a way to make FullScreenViewModal a little more "children-agnostic" since saveContentCallback and copyContentCallback are kind of media-specific and presumably FullScreenViewModal should be a general purpose component.

Accepting to unblock, but might be worth thinking through a more general purpose API?

Keeping constants that are imported from lots of different places in separate files also helps to avoid import cycles

Adding @ashoat as blocking since I don't have a ton of experience w/ Reanimated and had some issues w/ refactoring Reanimated code in the past (specifically https://phab.comm.dev/D9206#271158)

Makes sense, wonder if there's a way to make FullScreenViewModal a little more "children-agnostic" since saveContentCallback and copyContentCallback are kind of media-specific and presumably FullScreenViewModal should be a general purpose component.

Looks like a straightforward find+replace. Thanks for laying out the outline of the stack in the summary, made it easier to make sense of each diff

Seems reasonable

Double Extra Large??

In D9358#275100, @atul wrote:

In D9358#275095, @ginsu wrote:

Don't love the indirection, but seems like a reasonable approach

Wdym by indirection here? Happy to make any updates to make it less indirect

cc @atul

Just that the constants are in a separate file creates "distance" from where the values are defined and where they're used. But given you need to use them in multiple places this seems like a good bet.

IDE will "show" value of eg userProfileActionButtonHeight on hover anyways

We need to factor those parts out, so I created FullScreenViewModal which will be a generic component that can be used in both cases for image multimedia messages and user avatars.

In D9358#275095, @ginsu wrote:

Don't love the indirection, but seems like a reasonable approach

Wdym by indirection here? Happy to make any updates to make it less indirect

cc @atul

Looks good, let's memoize ImageModal before landing

Don't love the indirection, but seems like a reasonable approach

Don't love the indirection, but seems like a reasonable approach

dedup

simplify logic a little

update

update

Thanks @michal!

Thanks!! This looks clean, and I was able to confirm that it works on Safari version 15 running on macOS 12 via lambdatest.com