Page MenuHomePhabricator
Differential D14301

[native] Migrate buttons refs and measuring in FullScreenViewModal
ClosedPublic
Actions

Authored by angelika on Feb 6 2025, 5:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 14, 7:59 AM
Unknown Object (File)
Fri, Mar 14, 7:59 AM
Unknown Object (File)
Fri, Mar 14, 7:59 AM
Unknown Object (File)
Thu, Mar 13, 11:51 AM
Unknown Object (File)
Thu, Mar 13, 11:42 AM
Unknown Object (File)
Thu, Mar 13, 10:51 AM
Unknown Object (File)
Tue, Mar 11, 6:31 PM
Unknown Object (File)
Tue, Mar 11, 4:14 PM
View All 44 Files
Subscribers
ashoat

Details

Reviewers
tomek
ashoat
Commits
rCOMM86f44da46631: [native] Migrate buttons refs and measuring in FullScreenViewModal
Summary

https://linear.app/comm/issue/ENG-8150/convert-fullscreenviewmodal-to-reanimated-2-syntax

Migrate buttons refs and measuring them to the functional component. Also migrate outsideButtons function to reanimated v2 api. I also added handling safe area offsets like in the CameraModal migration.

At this point the component is broken and will be gradually fixed in the next diffs.

Depends on D14300

Test Plan

Tested after migration: verify the buttons work.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

angelika created this revision.Feb 6 2025, 5:07 PM
angelika held this revision as a draft.
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 6 2025, 5:07 PM
angelika added a child revision: D14302: [native] Migrate pan gesture handling in FullScreenViewModal.Feb 6 2025, 5:08 PM
Harbormaster completed remote builds in B33416: Diff 46945.Feb 6 2025, 5:37 PM
angelika edited the summary of this revision. (Show Details)Feb 7 2025, 5:19 AM
angelika updated this revision to Diff 47079.Feb 12 2025, 8:51 PM

Rebase & bugfixes

angelika published this revision for review.Feb 12 2025, 9:05 PM
Harbormaster completed remote builds in B33495: Diff 47079.Feb 12 2025, 9:31 PM
ashoat accepted this revision.Feb 14 2025, 12:32 AM
ashoat added inline comments.
native/components/full-screen-view-modal.react.js
1221–1222 ↗(On Diff #47079)

Do these checks work correctly when the viewer rotates the FullScreenViewModal to landscape? Curious if you tested that, and if the insets need to be applied in any of the other comparison operations

1223–1224 ↗(On Diff #47079)

Did you confirm that the y-coordinators were incorrect before applying the insets, and that applying the insets corrected them? (For example by pressing shortly above/below the buttons and noticing that the outsideButtons check worked incorrectly)

This revision is now accepted and ready to land.Feb 14 2025, 12:32 AM
angelika updated this revision to Diff 47183.Feb 20 2025, 2:12 PM

Rebase & bugfixes

angelika added inline comments.Feb 20 2025, 2:45 PM
native/components/full-screen-view-modal.react.js
1221–1222 ↗(On Diff #47079)

Haven't tested that, but after removing insets (see comment below) it works

1223–1224 ↗(On Diff #47079)

After more testing it seems like inset.top doesn't need to be accounted for here, and the outsideButtons function was broken because measure was returning incorrect values because there was no wrapper view like in original code. After adding wrapper view it's fixed.

ashoat added inline comments.Feb 20 2025, 9:28 PM
native/components/full-screen-view-modal.react.js
1223–1224 ↗(On Diff #47079)

Noticed there’s no new wrapper view in this revision. Was that added in a different diff? Or already present in a different diff? I hadn’t noticed any missing wrapper views in my prior review… it seemed like you had kept the same view hierarchy, but I may have missed something

ashoat added inline comments.Feb 20 2025, 9:58 PM
native/components/full-screen-view-modal.react.js
1223–1224 ↗(On Diff #47079)

Looks like a wrapper view was added in D14313, but I'm still a bit confused about the line "there was no wrapper view like in original code". Which one is the wrapper view in the original code?

angelika added inline comments.Feb 21 2025, 8:16 PM
native/components/full-screen-view-modal.react.js
1223–1224 ↗(On Diff #47079)

I added a view in this diff: https://phab.comm.dev/D14313, the outermost <Animated.View style={styles.container}> wrapping <GestureDetector gesture={gesture}>.
In the original code <Animated.View style={styles.container}> was wrapping every gesture handler other than PinchGestureHandler.

angelika updated this revision to Diff 47217.Feb 24 2025, 3:50 AM

Rebase

Harbormaster completed remote builds in B33597: Diff 47217.Feb 24 2025, 4:48 AM
Closed by commit rCOMM86f44da46631: [native] Migrate buttons refs and measuring in FullScreenViewModal (authored by angelika). · Explain WhyFeb 25 2025, 2:48 AM
This revision was automatically updated to reflect the committed changes.
angelika added a commit: rCOMM86f44da46631: [native] Migrate buttons refs and measuring in FullScreenViewModal.

Revision Contents
Changeset List

PathSize
native/
components/
196 lines

Diff 47245

View Options

native/components/full-screen-view-modal.react.js

// @flow // @flow
import t from 'tcomb'; import t from 'tcomb';
import type { TInterface, TUnion } from 'tcomb'; import type { TInterface, TUnion } from 'tcomb';
import { mixedRawThreadInfoValidator } from 'lib/permissions/minimally-encoded-thread-permissions-validators.js'; import { mixedRawThreadInfoValidator } from 'lib/permissions/minimally-encoded-raw-thread-info-validators.js';
import { mediaValidator } from 'lib/types/media-types.js'; import { mediaValidator } from 'lib/types/media-types.js';
import { import {
rawMessageInfoValidator, rawMessageInfoValidator,
messageTruncationStatusesValidator, messageTruncationStatusesValidator,
} from 'lib/types/message-types.js'; } from 'lib/types/message-types.js';
import { userSurfacedPermissionValidator } from 'lib/types/thread-permission-types.js'; import { userSurfacedPermissionValidator } from 'lib/types/thread-permission-types.js';
import { threadTypes } from 'lib/types/thread-types-enum.js'; import { threadTypes } from 'lib/types/thread-types-enum.js';
import { import {
▲ Show 20 Lines • Show All 325 Lines • Show Last 20 Lines