Skip to content
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

Fix popover menu anchor alignment prop #19849

Merged
merged 19 commits into from
Jun 9, 2023
Merged

Fix popover menu anchor alignment prop #19849

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
bbd951a
Fix avatar dropdown in profile and workspace setting
huzaifa-99 May 30, 2023
ad455dc
Fix dropdown position for workspace list 3-dots menu
huzaifa-99 May 30, 2023
3008412
Fix lint issues
huzaifa-99 May 30, 2023
4a3db13
Fix lint issues
huzaifa-99 May 30, 2023
51f8691
Fix lint issues
huzaifa-99 May 30, 2023
0554e58
Fix lint issues
huzaifa-99 May 30, 2023
a397e99
Merge branch 'main' into 19453-fix-popover-menu-anchor-alignment-prop
huzaifa-99 Jun 1, 2023
44e316e
Merge branch 'main' into 19453-fix-popover-menu-anchor-alignment-prop
huzaifa-99 Jun 5, 2023
c5b6dec
Added getVerticalOffset no-op to add vertical gap on desktop
huzaifa-99 Jun 6, 2023
0158ed7
Fix newline diff
huzaifa-99 Jun 6, 2023
a6e3d4c
Move desktop header gap to const
huzaifa-99 Jun 6, 2023
b7f0c1c
Renamed desktop header gap const
huzaifa-99 Jun 6, 2023
b65dd2a
Added js doc for getVerticalOffset
huzaifa-99 Jun 6, 2023
068c0b3
Merge branch 'main' into 19453-fix-popover-menu-anchor-alignment-prop
huzaifa-99 Jun 7, 2023
c0e29a3
Merge branch 'main' into 19453-fix-popover-menu-anchor-alignment-prop
huzaifa-99 Jun 7, 2023
22037c8
Merge branch 'main' into 19453-fix-popover-menu-anchor-alignment-prop
huzaifa-99 Jun 7, 2023
47b14b5
Merge branch 'main' into 19453-fix-popover-menu-anchor-alignment-prop
huzaifa-99 Jun 9, 2023
3476a4e
Fix merge issue
huzaifa-99 Jun 9, 2023
c2ce3bf
Renamed getVerticalOffset style to getPopOverVerticalOffset
huzaifa-99 Jun 9, 2023
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
1 change: 1 addition & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,7 @@ const CONST = {
WIDTH: 320,
HEIGHT: 416,
},
DESKTOP_HEADER_PADDING: 12,
CATEGORY_SHORTCUT_BAR_HEIGHT: 32,
SMALL_EMOJI_PICKER_SIZE: {
WIDTH: '100%',
Expand Down
13 changes: 13 additions & 0 deletions src/components/HeaderWithBackButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, {Component} from 'react';
import PropTypes from 'prop-types';
import {View, Keyboard} from 'react-native';
import _ from 'underscore';
import styles from '../styles/styles';
import Header from './Header';
import Navigation from '../libs/Navigation/Navigation';
Expand All @@ -18,6 +19,7 @@ import withKeyboardState, {keyboardStatePropTypes} from './withKeyboardState';
import AvatarWithDisplayName from './AvatarWithDisplayName';
import iouReportPropTypes from '../pages/iouReportPropTypes';
import participantPropTypes from './participantPropTypes';
import CONST from '../CONST';
import PressableWithoutFeedback from './Pressable/PressableWithoutFeedback';
import PinButton from './PinButton';

Expand Down Expand Up @@ -66,6 +68,12 @@ const propTypes = {
left: PropTypes.number,
}),

/** The anchor alignment of the menu */
threeDotsAnchorAlignment: PropTypes.shape({
horizontal: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL)),
vertical: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_VERTICAL)),
}),

/** Whether we should show a close button */
shouldShowCloseButton: PropTypes.bool,

Expand Down Expand Up @@ -135,6 +143,10 @@ const defaultProps = {
vertical: 0,
horizontal: 0,
},
threeDotsAnchorAlignment: {
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP,
},
};

class HeaderWithBackButton extends Component {
Expand Down Expand Up @@ -233,6 +245,7 @@ class HeaderWithBackButton extends Component {
menuItems={this.props.threeDotsMenuItems}
onIconPress={this.props.onThreeDotsButtonPress}
anchorPosition={this.props.threeDotsAnchorPosition}
anchorAlignment={this.props.threeDotsAnchorAlignment}
/>
)}

Expand Down
2 changes: 1 addition & 1 deletion src/components/PopoverMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const PopoverMenu = (props) => {
return (
<PopoverWithMeasuredContent
anchorPosition={props.anchorPosition}
anchorAlignment={props.anchorOrigin}
anchorAlignment={props.anchorAlignment}
onClose={props.onClose}
isVisible={props.isVisible}
onModalHide={() => {
Expand Down
13 changes: 13 additions & 0 deletions src/components/ThreeDotsMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import React, {Component} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
import Icon from '../Icon';
import PopoverMenu from '../PopoverMenu';
import styles from '../../styles/styles';
import withLocalize, {withLocalizePropTypes} from '../withLocalize';
import Tooltip from '../Tooltip';
import * as Expensicons from '../Icon/Expensicons';
import ThreeDotsMenuItemPropTypes from './ThreeDotsMenuItemPropTypes';
import CONST from '../../CONST';
import PressableWithoutFeedback from '../Pressable/PressableWithoutFeedback';

const propTypes = {
Expand Down Expand Up @@ -39,6 +41,12 @@ const propTypes = {
bottom: PropTypes.number,
left: PropTypes.number,
}).isRequired,

/** The anchor alignment of the menu */
anchorAlignment: PropTypes.shape({
horizontal: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL)),
vertical: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_VERTICAL)),
}),
};

const defaultProps = {
Expand All @@ -47,6 +55,10 @@ const defaultProps = {
iconStyles: [],
icon: Expensicons.ThreeDots,
onIconPress: () => {},
anchorAlignment: {
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
},
};

class ThreeDotsMenu extends Component {
Expand Down Expand Up @@ -97,6 +109,7 @@ class ThreeDotsMenu extends Component {
onClose={this.hidePopoverMenu}
isVisible={this.state.isPopupMenuVisible}
anchorPosition={this.props.anchorPosition}
anchorAlignment={this.props.anchorAlignment}
onItemSelected={this.hidePopoverMenu}
menuItems={this.props.menuItems}
/>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Profile/ProfilePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ const ProfilePage = (props) => {
onImageSelected={PersonalDetails.updateAvatar}
onImageRemoved={PersonalDetails.deleteAvatar}
anchorPosition={styles.createMenuPositionProfile(props.windowWidth)}
anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.CENTER, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP}}
anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP}}
size={CONST.AVATAR_SIZE.LARGE}
/>
</OfflineWithFeedback>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/WorkspaceSettingsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ function WorkspaceSettingsPage(props) {
fallbackIcon={Expensicons.FallbackWorkspaceAvatar}
style={[styles.mb3]}
anchorPosition={styles.createMenuPositionProfile(props.windowWidth)}
anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.CENTER, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP}}
anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP}}
isUsingDefaultAvatar={!lodashGet(props.policy, 'avatar', null)}
onImageSelected={(file) => Policy.updateWorkspaceAvatar(lodashGet(props.policy, 'id', ''), file)}
onImageRemoved={() => Policy.deleteWorkspaceAvatar(lodashGet(props.policy, 'id', ''))}
Expand Down
13 changes: 13 additions & 0 deletions src/styles/getVerticalOffset/index.desktop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import CONST from '../../CONST';
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment related to the whole directory, not this specific line.
Can we make the name of this function be more clear? Like can we add it under a directory that indicates what components use this or add something to the name or something?
Reading getVerticalOffset tells you nothing about what vertical offset is this supposed to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we make the name of this function be more clear? Like can we add it under a directory that indicates what components use this or add something to the name or something?

I think the one's directly in /styles/ folder are for these one-off cases like cardStyles, addOutlineWidth, editedLabelStyles and more, and those in styles/utilities are proper style utilities like cursor, visibility etc. (By one-off cases i meant the one we have in PR)

But i maybe wrong here since we also have bold and italic directly in /styles and not in /styles/utilities and vice versa. TBH idk why we have the styles/utilities in the first place it just seems unnecessary or if it is necessary then the styles folder's outside of it seems unnecessary.

Reading getVerticalOffset tells you nothing about what vertical offset is this supposed to be.

Its a little ambiguous yes, maybe we could add a direction with it like getVerticalOffsetFromTop etc or any other name, @Santhosh-Sellavel curious if you have any suggestions here?

cc: @iwiznia @Santhosh-Sellavel

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not being clear. This style is related to the PopOver component, no?
threeDotsPopoverOffset and threeDotsPopoverOffsetNoCloseButton then createMenuPositionProfile also ends up passing this to a PopOver component.
So, a better name might be getPopOverVerticalOffset or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, Thank you! I will be updating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iwiznia added those changes


/**
* Adds the header padding with vertical offset on desktop
* @param {Number} vertical
* @returns {Object}
*/
export default (vertical) => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add JS Doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// We add CONST.DESKTOP_HEADER_GAP on desktop which we
// need to add to vertical offset to have proper vertical
// offset on desktop
vertical: vertical + CONST.DESKTOP_HEADER_PADDING,
});
1 change: 1 addition & 0 deletions src/styles/getVerticalOffset/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default (vertical) => ({vertical});
9 changes: 5 additions & 4 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import writingDirection from './utilities/writingDirection';
import optionAlternateTextPlatformStyles from './optionAlternateTextPlatformStyles';
import pointerEventsNone from './pointerEventsNone';
import pointerEventsAuto from './pointerEventsAuto';
import getVerticalOffset from './getVerticalOffset';
import overflowXHidden from './overflowXHidden';
import CONST from '../CONST';
import cursor from './utilities/cursor';
Expand Down Expand Up @@ -768,7 +769,7 @@ const styles = {
},

headerGap: {
height: 12,
height: CONST.DESKTOP_HEADER_PADDING,
},

pushTextRight: {
Expand Down Expand Up @@ -1272,7 +1273,7 @@ const styles = {

createMenuPositionProfile: (windowWidth) => ({
horizontal: windowWidth - 355,
vertical: 250,
...getVerticalOffset(162),
}),

createMenuPositionReportActionCompose: (windowHeight) => ({
Expand Down Expand Up @@ -2979,12 +2980,12 @@ const styles = {
},

threeDotsPopoverOffset: (windowWidth) => ({
vertical: 50,
...getVerticalOffset(60),
horizontal: windowWidth - 60,
}),

threeDotsPopoverOffsetNoCloseButton: (windowWidth) => ({
vertical: 50,
...getVerticalOffset(60),
horizontal: windowWidth - 10,
}),

Expand Down