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)
Tue, Apr 15, 4:08 PM
Unknown Object (File)
Tue, Apr 15, 9:40 AM
Unknown Object (File)
Tue, Apr 15, 4:23 AM
Unknown Object (File)
Tue, Apr 15, 3:07 AM
Unknown Object (File)
Tue, Apr 15, 12:18 AM
Unknown Object (File)
Mon, Apr 14, 9:47 PM
Unknown Object (File)
Mon, Apr 14, 5:53 AM
Unknown Object (File)
Mar 14 2025, 7:59 AM
View All 51 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
No Lint Coverage
Unit
No Test Coverage

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 47183

View Options

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

Show First 20 Lines • Show All 155 Lines • ▼ Show 20 Lines
}; };
type Props = { type Props = {
...BaseProps, ...BaseProps,
// Redux state // Redux state
+dimensions: DerivedDimensionsInfo, +dimensions: DerivedDimensionsInfo,
// withOverlayContext // withOverlayContext
+overlayContext: ?OverlayContextType, +overlayContext: ?OverlayContextType,
+isActive: boolean, +isActive: boolean,
};
type State = {
+closeButtonEnabled: boolean, +closeButtonEnabled: boolean,
+actionLinksEnabled: boolean, +actionLinksEnabled: boolean,
}; +updateCloseButtonEnabled: ([number]) => void,
class FullScreenViewModal extends React.PureComponent<Props, State> { +updateActionLinksEnabled: ([number]) => void,
state: State = {
closeButtonEnabled: true,
actionLinksEnabled: true,
}; };
class FullScreenViewModal extends React.PureComponent<Props> {
closeButton: ?React.ElementRef<TouchableOpacityInstance>; closeButton: ?React.ElementRef<TouchableOpacityInstance>;
mediaIconsContainer: ?React.ElementRef<typeof View>; mediaIconsContainer: ?React.ElementRef<typeof View>;
closeButtonX: Value = new Value(-1); closeButtonX: Value = new Value(-1);
closeButtonY: Value = new Value(-1); closeButtonY: Value = new Value(-1);
closeButtonWidth: Value = new Value(0); closeButtonWidth: Value = new Value(0);
closeButtonHeight: Value = new Value(0); closeButtonHeight: Value = new Value(0);
closeButtonLastState: Value = new Value(1); closeButtonLastState: Value = new Value(1);
mediaIconsX: Value = new Value(-1); mediaIconsX: Value = new Value(-1);
▲ Show 20 Lines • Show All 472 Lines • ▼ Show 20 Lines return block([
], ],
), ),
), ),
set(this.actionLinksLastState, actionLinksState), set(this.actionLinksLastState, actionLinksState),
set(this.closeButtonLastState, closeButtonState), set(this.closeButtonLastState, closeButtonState),
set(wasZoomed, isZoomed), set(wasZoomed, isZoomed),
set(lastTapX, singleTapX), set(lastTapX, singleTapX),
set(lastTapY, singleTapY), set(lastTapY, singleTapY),
call([eq(curCloseButtonOpacity, 1)], this.setCloseButtonEnabled), call([eq(curCloseButtonOpacity, 1)], this.props.updateCloseButtonEnabled),
call([eq(curActionLinksOpacity, 1)], this.setActionLinksEnabled), call([eq(curActionLinksOpacity, 1)], this.props.updateActionLinksEnabled),
]); ]);
} }
doubleTapUpdate( doubleTapUpdate(
// Inputs // Inputs
doubleTapState: Node, doubleTapState: Node,
doubleTapX: Node, doubleTapX: Node,
doubleTapY: Node, doubleTapY: Node,
▲ Show 20 Lines • Show All 316 Lines • ▼ Show 20 Lines const mediaIconsButtonStyle = {
opacity: this.actionLinksOpacity, opacity: this.actionLinksOpacity,
}; };
let saveButton; let saveButton;
if (saveContentCallback) { if (saveContentCallback) {
saveButton = ( saveButton = (
<TouchableOpacity <TouchableOpacity
onPress={saveContentCallback} onPress={saveContentCallback}
disabled={!this.state.actionLinksEnabled} disabled={!this.props.actionLinksEnabled}
style={styles.mediaIconButtons} style={styles.mediaIconButtons}
> >
<SWMansionIcon name="save" style={styles.mediaIcon} /> <SWMansionIcon name="save" style={styles.mediaIcon} />
<Text style={styles.mediaIconText}>Save</Text> <Text style={styles.mediaIconText}>Save</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
let copyButton; let copyButton;
if (Platform.OS === 'ios' && copyContentCallback) { if (Platform.OS === 'ios' && copyContentCallback) {
copyButton = ( copyButton = (
<TouchableOpacity <TouchableOpacity
onPress={copyContentCallback} onPress={copyContentCallback}
disabled={!this.state.actionLinksEnabled} disabled={!this.props.actionLinksEnabled}
style={styles.mediaIconButtons} style={styles.mediaIconButtons}
> >
<SWMansionIcon name="copy" style={styles.mediaIcon} /> <SWMansionIcon name="copy" style={styles.mediaIcon} />
<Text style={styles.mediaIconText}>Copy</Text> <Text style={styles.mediaIconText}>Copy</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
Show All 26 Lines const view = (
</View> </View>
<SafeAreaView style={styles.buttonsOverlay}> <SafeAreaView style={styles.buttonsOverlay}>
<View style={styles.fill}> <View style={styles.fill}>
<Animated.View <Animated.View
style={[styles.closeButtonContainer, closeButtonStyle]} style={[styles.closeButtonContainer, closeButtonStyle]}
> >
<TouchableOpacity <TouchableOpacity
onPress={this.close} onPress={this.close}
disabled={!this.state.closeButtonEnabled} disabled={!this.props.closeButtonEnabled}
onLayout={this.onCloseButtonLayout} onLayout={this.onCloseButtonLayout}
ref={this.closeButtonRef} ref={this.closeButtonRef}
> >
<Text style={styles.closeButton}>×</Text> <Text style={styles.closeButton}>×</Text>
</TouchableOpacity> </TouchableOpacity>
</Animated.View> </Animated.View>
{mediaActionButtons} {mediaActionButtons}
</View> </View>
▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Lines return (
</PinchGestureHandler> </PinchGestureHandler>
); );
} }
close = () => { close = () => {
this.props.navigation.goBackOnce(); this.props.navigation.goBackOnce();
}; };
setCloseButtonEnabled = ([enabledNum]: [number]) => {
const enabled = !!enabledNum;
if (this.state.closeButtonEnabled !== enabled) {
this.setState({ closeButtonEnabled: enabled });
}
};
setActionLinksEnabled = ([enabledNum]: [number]) => {
const enabled = !!enabledNum;
if (this.state.actionLinksEnabled !== enabled) {
this.setState({ actionLinksEnabled: enabled });
}
};
closeButtonRef = ( closeButtonRef = (
closeButton: ?React.ElementRef<typeof TouchableOpacity>, closeButton: ?React.ElementRef<typeof TouchableOpacity>,
) => { ) => {
this.closeButton = (closeButton: any); this.closeButton = (closeButton: any);
}; };
mediaIconsRef = (mediaIconsContainer: ?React.ElementRef<typeof View>) => { mediaIconsRef = (mediaIconsContainer: ?React.ElementRef<typeof View>) => {
this.mediaIconsContainer = mediaIconsContainer; this.mediaIconsContainer = mediaIconsContainer;
▲ Show 20 Lines • Show All 116 Lines • ▼ Show 20 Lines ) {
}, [isActive]); }, [isActive]);
React.useEffect(() => { React.useEffect(() => {
return () => { return () => {
Orientation.lockToPortrait(); Orientation.lockToPortrait();
}; };
}, []); }, []);
const [closeButtonEnabled, setCloseButtonEnabled] = React.useState(true);
const [actionLinksEnabled, setActionLinksEnabled] = React.useState(true);
const updateCloseButtonEnabled = React.useCallback(
([enabledNum]: [number]) => {
tomekUnsubmitted
Not Done
Inline Actions

We should prefer Array type

tomek: We should prefer `Array` type
angelikaAuthorUnsubmitted
Done
Inline Actions

This was just copied from the original code and the [number] argument will be replaced with number argument in https://phab.comm.dev/D14304 anyway

angelika: This was just copied from the original code and the `[number]` argument will be replaced with…
const enabled = !!enabledNum;
if (closeButtonEnabled !== enabled) {
setCloseButtonEnabled(enabled);
}
},
[closeButtonEnabled],
);
const updateActionLinksEnabled = React.useCallback(
([enabledNum]: [number]) => {
const enabled = !!enabledNum;
if (actionLinksEnabled !== enabled) {
setActionLinksEnabled(enabled);
}
},
[actionLinksEnabled],
);
return ( return (
<FullScreenViewModal <FullScreenViewModal
{...props} {...props}
dimensions={dimensions} dimensions={dimensions}
overlayContext={overlayContext} overlayContext={overlayContext}
isActive={isActive} isActive={isActive}
closeButtonEnabled={closeButtonEnabled}
actionLinksEnabled={actionLinksEnabled}
updateCloseButtonEnabled={updateCloseButtonEnabled}
updateActionLinksEnabled={updateActionLinksEnabled}
/> />
); );
}); });
export default ConnectedFullScreenViewModal; export default ConnectedFullScreenViewModal;