Skip to content

Fix wrong popover position #57920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 3, 2025
Merged

Fix wrong popover position #57920

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9811f2e
fix wrong popover position
bernhardoj Mar 6, 2025
fd41d62
lint
bernhardoj Mar 6, 2025
8302ac1
lint
bernhardoj Mar 6, 2025
544b47f
Merge branch 'main' into fix/57011-modal-opens-at-bottom-page
bernhardoj Mar 20, 2025
868fdef
revert unexpected changes
bernhardoj Mar 31, 2025
67eee8b
update type
bernhardoj Mar 31, 2025
a90224f
Merge branch 'main' into fix/57011-modal-opens-at-bottom-page
bernhardoj Mar 31, 2025
4d9339b
Merge branch 'main' into fix/57011-modal-opens-at-bottom-page
bernhardoj Apr 3, 2025
61ed05e
prettier
bernhardoj Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/components/ThreeDotsMenu/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import useLocalize from '@hooks/useLocalize';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import {isMobile} from '@libs/Browser';
import type {AnchorPosition} from '@styles/index';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand All @@ -29,6 +30,7 @@ function ThreeDotsMenu({
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP, // we assume that popover menu opens below the button, anchor is at TOP
},
getAnchorPosition,
shouldOverlay = false,
shouldSetModalVisibility = true,
disabled = false,
Expand All @@ -42,6 +44,7 @@ function ThreeDotsMenu({
const theme = useTheme();
const styles = useThemeStyles();
const [isPopupMenuVisible, setPopupMenuVisible] = useState(false);
const [position, setPosition] = useState<AnchorPosition>();
const buttonRef = useRef<View>(null);
const {translate} = useLocalize();
const isBehindModal = modal?.willAlertModalBecomeVisible && !modal?.isPopover && !shouldOverlay;
Expand All @@ -68,10 +71,17 @@ function ThreeDotsMenu({
}
hideProductTrainingTooltip?.();
buttonRef.current?.blur();
showPopoverMenu();
if (onIconPress) {
onIconPress();

if (getAnchorPosition) {
getAnchorPosition().then((value) => {
setPosition(value);
showPopoverMenu();
});
} else {
showPopoverMenu();
}

onIconPress?.();
};

const TooltipToRender = shouldShowProductTrainingTooltip ? EducationalTooltip : Tooltip;
Expand Down Expand Up @@ -121,7 +131,7 @@ function ThreeDotsMenu({
<PopoverMenu
onClose={hidePopoverMenu}
isVisible={isPopupMenuVisible && !isBehindModal}
anchorPosition={anchorPosition}
anchorPosition={position ?? anchorPosition ?? {horizontal: 0, vertical: 0}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we passing both position and anchorPosition, when we already passing the anchorPosition callback?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pass {horizontal: 0, vertical: 0} as default value to position state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a component passes anchorPosition and not getAnchorPosition, then the position will be undefined and fallback to anchorPosition props.

anchorAlignment={anchorAlignment}
onItemSelected={hidePopoverMenu}
menuItems={menuItems}
Expand Down
24 changes: 17 additions & 7 deletions src/components/ThreeDotsMenu/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type {LayoutRectangle, NativeSyntheticEvent, StyleProp, ViewStyle} from 'react-native';
import type {StyleProp, ViewStyle} from 'react-native';
import type {PopoverMenuItem} from '@components/PopoverMenu';
import type {TranslationPaths} from '@src/languages/types';
import type {AnchorPosition} from '@src/styles';
Expand All @@ -24,9 +24,6 @@ type ThreeDotsMenuProps = {
/** menuItems that'll show up on toggle of the popup menu */
menuItems: PopoverMenuItem[];

/** The anchor position of the menu */
anchorPosition: AnchorPosition;

/** The anchor alignment of the menu */
anchorAlignment?: AnchorAlignment;

Expand All @@ -52,7 +49,20 @@ type ThreeDotsMenuProps = {
isNested?: boolean;
};

type LayoutChangeEventWithTarget = NativeSyntheticEvent<{layout: LayoutRectangle; target: HTMLElement}>;
type ThreeDotsMenuWithOptionalAnchorProps =
| (ThreeDotsMenuProps & {
/** The anchor position of the menu */
anchorPosition: AnchorPosition;

/** A callback to get the anchor position dynamically */
getAnchorPosition?: () => Promise<AnchorPosition>;
})
| (ThreeDotsMenuProps & {
/** The anchor position of the menu */
anchorPosition?: AnchorPosition;

/** A callback to get the anchor position dynamically */
getAnchorPosition: () => Promise<AnchorPosition>;
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between both types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First type requires anchorPosition, second type requires getAnchorPosition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's choose one. I think getAnchorPosition is good enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj Is there a need for both props? Should we just keep one getAnchorPosition? If a user wants to send static position they can do this via this callback as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need both. With this type, the user needs to either pass anchorPosition or getAnchorPosition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAnchorPosition={() => Promise.resolve(threeDotsAnchorPosition)} I believe this.

Hmm, yeah it is a little overhead of promise but having two props for same thing while a single prop can handle both cases is confusing. While you must have noticed, we have a few components which are consuming a huge number of props making is very confusing on how these components can be utilized

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some components have too many props because they're too big, trying to handle many cases/responsibilities.

If we use getAnchorPosition only, it will:

  1. add delay (because of promise)
  2. extra render (because of set state)

for component that already has the anchor position information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I am fine with it. It's not a blocker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, should the user pass only one or both can be passed?

In case, only one should be set. We can update the type to

Suggested change
type ThreeDotsMenuWithOptionalAnchorProps =
| (ThreeDotsMenuProps & {
/** The anchor position of the menu */
anchorPosition: AnchorPosition;
/** A callback to get the anchor position dynamically */
getAnchorPosition?: () => Promise<AnchorPosition>;
})
| (ThreeDotsMenuProps & {
/** The anchor position of the menu */
anchorPosition?: AnchorPosition;
/** A callback to get the anchor position dynamically */
getAnchorPosition: () => Promise<AnchorPosition>;
});
type ThreeDotsMenuWithOptionalAnchorProps =
| (ThreeDotsMenuProps & {
/** The anchor position of the menu */
anchorPosition: AnchorPosition;
/** A callback to get the anchor position dynamically */
getAnchorPosition?: never;
})
| (ThreeDotsMenuProps & {
/** The anchor position of the menu */
anchorPosition?: never;
/** A callback to get the anchor position dynamically */
getAnchorPosition: () => Promise<AnchorPosition>;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

export type {LayoutChangeEventWithTarget};
export default ThreeDotsMenuProps;
export default ThreeDotsMenuWithOptionalAnchorProps;
35 changes: 20 additions & 15 deletions src/pages/Search/SavedSearchItemThreeDotMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import React, {useRef, useState} from 'react';
import type {LayoutChangeEvent, LayoutRectangle, NativeSyntheticEvent} from 'react-native';
import React, {useCallback, useRef} from 'react';
import {View} from 'react-native';
import type {PopoverMenuItem} from '@components/PopoverMenu';
import ThreeDotsMenu from '@components/ThreeDotsMenu';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useThemeStyles from '@hooks/useThemeStyles';
import type {AnchorPosition} from '@styles/index';
import CONST from '@src/CONST';

type SavedSearchItemThreeDotMenuProps = {
Expand All @@ -14,29 +15,33 @@ type SavedSearchItemThreeDotMenuProps = {
shouldRenderTooltip: boolean;
};

type LayoutChangeEventWithTarget = NativeSyntheticEvent<{layout: LayoutRectangle; target: HTMLElement}>;

function SavedSearchItemThreeDotMenu({menuItems, isDisabledItem, hideProductTrainingTooltip, renderTooltipContent, shouldRenderTooltip}: SavedSearchItemThreeDotMenuProps) {
const threeDotsMenuContainerRef = useRef<View>(null);
const [threeDotsMenuPosition, setThreeDotsMenuPosition] = useState({horizontal: 0, vertical: 0});
const {shouldUseNarrowLayout} = useResponsiveLayout();
const styles = useThemeStyles();

const calculateAndSetThreeDotsMenuPosition = useCallback(() => {
if (shouldUseNarrowLayout) {
return Promise.resolve({horizontal: 0, vertical: 0});
}
return new Promise<AnchorPosition>((resolve) => {
threeDotsMenuContainerRef.current?.measureInWindow((x, y, width) => {
resolve({
horizontal: x + width,
vertical: y,
});
});
});
}, [shouldUseNarrowLayout]);

return (
<View
ref={threeDotsMenuContainerRef}
style={[isDisabledItem && styles.pointerEventsNone]}
onLayout={(e: LayoutChangeEvent) => {
const target = e.target || (e as LayoutChangeEventWithTarget).nativeEvent.target;
target?.measureInWindow((x, y, width) => {
setThreeDotsMenuPosition({
horizontal: x + width,
vertical: y,
});
});
}}
>
<ThreeDotsMenu
menuItems={menuItems}
anchorPosition={threeDotsMenuPosition}
getAnchorPosition={calculateAndSetThreeDotsMenuPosition}
renderProductTrainingTooltipContent={renderTooltipContent}
shouldShowProductTrainingTooltip={shouldRenderTooltip}
anchorAlignment={{
Expand Down
37 changes: 17 additions & 20 deletions src/pages/settings/Subscription/CardSection/CardSectionActions/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import React, {useMemo, useRef, useState} from 'react';
import type {LayoutChangeEvent} from 'react-native';
import React, {useCallback, useMemo, useRef} from 'react';
import {View} from 'react-native';
import * as Expensicons from '@components/Icon/Expensicons';
import ThreeDotsMenu from '@components/ThreeDotsMenu';
import type ThreeDotsMenuProps from '@components/ThreeDotsMenu/types';
import type {LayoutChangeEventWithTarget} from '@components/ThreeDotsMenu/types';
import useLocalize from '@hooks/useLocalize';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import Navigation from '@navigation/Navigation';
Expand All @@ -20,7 +18,6 @@ const anchorAlignment = {
function CardSectionActions() {
const {shouldUseNarrowLayout} = useResponsiveLayout();
const {translate} = useLocalize();
const [threeDotsMenuPosition, setThreeDotsMenuPosition] = useState<AnchorPosition>({horizontal: 0, vertical: 0});
const threeDotsMenuContainerRef = useRef<View>(null);

const overflowMenu: ThreeDotsMenuProps['menuItems'] = useMemo(
Expand All @@ -39,25 +36,25 @@ function CardSectionActions() {
[translate],
);

return (
<View
ref={threeDotsMenuContainerRef}
onLayout={(e: LayoutChangeEvent) => {
if (shouldUseNarrowLayout) {
return;
}
const target = e.target || (e as LayoutChangeEventWithTarget).nativeEvent.target;
target?.measureInWindow((x, y, width) => {
setThreeDotsMenuPosition({
horizontal: x + width,
vertical: y,
});
const calculateAndSetThreeDotsMenuPosition = useCallback(() => {
if (shouldUseNarrowLayout) {
return Promise.resolve({horizontal: 0, vertical: 0});
}
return new Promise<AnchorPosition>((resolve) => {
threeDotsMenuContainerRef.current?.measureInWindow((x, y, width, height) => {
resolve({
horizontal: x + width,
vertical: y + height,
});
}}
>
});
});
}, [shouldUseNarrowLayout]);

return (
<View ref={threeDotsMenuContainerRef}>
<ThreeDotsMenu
getAnchorPosition={calculateAndSetThreeDotsMenuPosition}
menuItems={overflowMenu}
anchorPosition={threeDotsMenuPosition}
anchorAlignment={anchorAlignment}
shouldOverlay
/>
Expand Down
10 changes: 4 additions & 6 deletions src/pages/settings/Subscription/SubscriptionDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import Text from '@components/Text';
import useLocalize from '@hooks/useLocalize';
import useThemeIllustrations from '@hooks/useThemeIllustrations';
import useThemeStyles from '@hooks/useThemeStyles';
import {clearUpdateSubscriptionSizeError, updateSubscriptionType} from '@libs/actions/Subscription';
import Navigation from '@libs/Navigation/Navigation';
import TaxExemptActions from '@pages/settings/Subscription/TaxExemptActions';
import variables from '@styles/variables';
import * as Subscription from '@userActions/Subscription';
import type {SubscriptionType} from '@src/CONST';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand Down Expand Up @@ -57,7 +57,7 @@ function SubscriptionDetails() {
return;
}

Subscription.updateSubscriptionType(option);
updateSubscriptionType(option);
};

const onSubscriptionSizePress = () => {
Expand All @@ -75,9 +75,7 @@ function SubscriptionDetails() {
<OfflineWithFeedback
pendingAction={privateSubscription?.pendingFields?.userCount}
errors={privateSubscription?.errorFields?.userCount}
onClose={() => {
Subscription.clearUpdateSubscriptionSizeError();
}}
onClose={clearUpdateSubscriptionSizeError}
>
<MenuItemWithTopDescription
description={translate('subscription.details.subscriptionSize')}
Expand Down Expand Up @@ -107,7 +105,7 @@ function SubscriptionDetails() {
/>
)}
</View>
{!privateTaxExempt && <TaxExemptActions />}
{true && <TaxExemptActions />}
</View>
{!!account?.isApprovedAccountant || !!account?.isApprovedAccountantClient ? (
<View style={[styles.borderedContentCard, styles.p5, styles.mt5]}>
Expand Down
33 changes: 16 additions & 17 deletions src/pages/settings/Subscription/TaxExemptActions/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import React, {useMemo, useRef, useState} from 'react';
import React, {useCallback, useMemo, useRef} from 'react';
import {View} from 'react-native';
import type {LayoutChangeEvent} from 'react-native';
import * as Expensicons from '@components/Icon/Expensicons';
import ThreeDotsMenu from '@components/ThreeDotsMenu';
import type ThreeDotsMenuProps from '@components/ThreeDotsMenu/types';
import type {LayoutChangeEventWithTarget} from '@components/ThreeDotsMenu/types';
import useLocalize from '@hooks/useLocalize';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -22,7 +20,6 @@ function TaxExemptActions() {
const {shouldUseNarrowLayout} = useResponsiveLayout();
const styles = useThemeStyles();
const {translate} = useLocalize();
const [threeDotsMenuPosition, setThreeDotsMenuPosition] = useState<AnchorPosition>({horizontal: 0, vertical: 0});
const threeDotsMenuContainerRef = useRef<View>(null);

const overflowMenu: ThreeDotsMenuProps['menuItems'] = useMemo(
Expand All @@ -40,26 +37,28 @@ function TaxExemptActions() {
[translate],
);

const calculateAndSetThreeDotsMenuPosition = useCallback(() => {
if (shouldUseNarrowLayout) {
return Promise.resolve({horizontal: 0, vertical: 0});
}
return new Promise<AnchorPosition>((resolve) => {
threeDotsMenuContainerRef.current?.measureInWindow((x, y, width, height) => {
resolve({
horizontal: x + width,
vertical: y + height,
});
});
});
}, [shouldUseNarrowLayout]);

return (
<View
ref={threeDotsMenuContainerRef}
style={[styles.mtn2, styles.pAbsolute, styles.rn3]}
onLayout={(e: LayoutChangeEvent) => {
if (shouldUseNarrowLayout) {
return;
}
const target = e.target || (e as LayoutChangeEventWithTarget).nativeEvent.target;
target?.measureInWindow((x, y, width) => {
setThreeDotsMenuPosition({
horizontal: x + width,
vertical: y,
});
});
}}
>
<ThreeDotsMenu
getAnchorPosition={calculateAndSetThreeDotsMenuPosition}
menuItems={overflowMenu}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we calculating it from the outside View but not from inside the ThreeDotsMenu?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i notice that not all 3 dot has the same calculation. saved search 3 dot for example doesn't add height to the vertical position.

vertical: y

anchorPosition={threeDotsMenuPosition}
anchorAlignment={anchorAlignment}
shouldOverlay
/>
Expand Down
38 changes: 18 additions & 20 deletions src/pages/workspace/WorkspacesListRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, {useRef, useState} from 'react';
import React, {useCallback, useRef} from 'react';
import {View} from 'react-native';
import type {LayoutChangeEvent, StyleProp, ViewStyle} from 'react-native';
import type {StyleProp, ViewStyle} from 'react-native';
import type {ValueOf} from 'type-fest';
import Avatar from '@components/Avatar';
import Badge from '@components/Badge';
Expand All @@ -10,7 +10,6 @@ import * as Illustrations from '@components/Icon/Illustrations';
import type {PopoverMenuItem} from '@components/PopoverMenu';
import Text from '@components/Text';
import ThreeDotsMenu from '@components/ThreeDotsMenu';
import type {LayoutChangeEventWithTarget} from '@components/ThreeDotsMenu/types';
import Tooltip from '@components/Tooltip';
import type {WithCurrentUserPersonalDetailsProps} from '@components/withCurrentUserPersonalDetails';
import withCurrentUserPersonalDetails from '@components/withCurrentUserPersonalDetails';
Expand Down Expand Up @@ -117,12 +116,25 @@ function WorkspacesListRow({
}: WorkspacesListRowProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const [threeDotsMenuPosition, setThreeDotsMenuPosition] = useState<AnchorPosition>({horizontal: 0, vertical: 0});
const threeDotsMenuContainerRef = useRef<View>(null);
const {shouldUseNarrowLayout} = useResponsiveLayout();

const ownerDetails = ownerAccountID && getPersonalDetailsByIDs({accountIDs: [ownerAccountID], currentUserAccountID: currentUserPersonalDetails.accountID}).at(0);

const calculateAndSetThreeDotsMenuPosition = useCallback(() => {
if (shouldUseNarrowLayout) {
return Promise.resolve({horizontal: 0, vertical: 0});
}
return new Promise<AnchorPosition>((resolve) => {
threeDotsMenuContainerRef.current?.measureInWindow((x, y, width, height) => {
resolve({
horizontal: x + width,
vertical: y + height,
});
});
});
}, [shouldUseNarrowLayout]);

if (layoutWidth === CONST.LAYOUT_WIDTH.NONE) {
// To prevent layout from jumping or rendering for a split second, when
// isWide is undefined we don't assume anything and simply return null.
Expand Down Expand Up @@ -166,24 +178,10 @@ function WorkspacesListRow({
<View style={[styles.flexRow, styles.gap2, styles.alignItemsCenter, isNarrow && styles.workspaceListRBR]}>
<BrickRoadIndicatorIcon brickRoadIndicator={brickRoadIndicator} />
</View>
<View
ref={threeDotsMenuContainerRef}
onLayout={(e: LayoutChangeEvent) => {
if (shouldUseNarrowLayout) {
return;
}
const target = e.target || (e as LayoutChangeEventWithTarget).nativeEvent.target;
target?.measureInWindow((x, y, width) => {
setThreeDotsMenuPosition({
horizontal: x + width,
vertical: y,
});
});
}}
>
<View ref={threeDotsMenuContainerRef}>
<ThreeDotsMenu
getAnchorPosition={calculateAndSetThreeDotsMenuPosition}
menuItems={menuItems}
anchorPosition={threeDotsMenuPosition}
anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.RIGHT, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP}}
shouldOverlay
disabled={shouldDisableThreeDotsMenu}
Expand Down
Loading
Loading