Popover fixes on tablet #7985

srdjanrist · 2022-03-02T18:02:24Z

Details

I have changed how popovers are shown on iPad medium and large devices. Since these are considered as native, sometimes popovers were shown bottom docked modals, instead of onscreen modals. Medium and large devices are considered as being "not-small" and this flag (of not being small) with a combination of native methods caused inconsistencies in different places that popovers are shown.

Fixed Issues

$ #7375

Tests

Click plus on list of all chats
Observe popover behavior

Open single chat
Click plus beside compose message
Observe popover behavior

Follow previous steps
Click attachment
Observe popover behavior

Open workspaces
Click three dots
Observe popover behavior

Open single workspace
Click on image
Observe popover behavior

Open user settings
Click on image to change it
Observe popover behavior

Open user settings
Click on payments
Click on "Add payment method"
Observe popover behavior

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Click plus on list of all chats
Verify that popover appears as modal on wide screens, and bottom docked on narrow screens

Open single chat
Click plus beside compose message
Verify that popover appears as modal on wide screens, and bottom docked on narrow screens

Follow previous steps
Click attachment
Verify that popover appears as modal on wide screens, and bottom docked on narrow screens

Open workspaces
Click three dots
Verify that popover appears as modal on wide screens, and bottom docked on narrow screens

Open single workspace
Click on image
Verify that popover appears as modal on wide screens, and bottom docked on narrow screens

Open user settings
Click on image to change it
Verify that popover appears as modal on wide screens, and bottom docked on narrow screens

Open user settings
Click on payments
Click on "Add payment method"
Verify that popover appears as modal on wide screens, and bottom docked on narrow screens

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

_

_

_

_

_

Mobile web

_

_

_

_

_

_

Desktop

_

_

_

_

iOS

iPad Narrow

iPad Wide

Android

…ection depending on screen size

…ative devices

parasharrajat

Missing QA Steps.

Fix it please.

srdjanrist · 2022-03-03T12:18:16Z

Missing QA Steps.

Fix it please.

I have updated QA steps.

parasharrajat · 2022-03-03T15:09:56Z

This is still not fixed. Do not use MD link format. Directly use the link as $ https://github.com/Expensify/App/issues/7375

srdjanrist · 2022-03-03T15:31:14Z

This is still not fixed. Do not use MD link format. Directly use the link as $ https://github.com/Expensify/App/issues/7375

Updated.

parasharrajat · 2022-03-03T16:32:30Z

Thanks for the hard work on the PR @srdjanrist. Could you please help me understand the motive and your approach behind the changes you have done?

There are a lot of images. Please use


<details>
<summary>Details</summary>



</details>

to group them. Thanks.

srdjanrist · 2022-03-04T09:25:09Z

Thanks for the hard work on the PR @srdjanrist. Could you please help me understand the motive and your approach behind the changes you have done?

Looking into component Popover native implementation, you can see on line 20 and 21, that depending on fromSidebarMediumScreen prop, either popover modal or bottom docked modal is used.
This issue occurs on iPads with diagonal greater than 9.7", and they are considered medium screen-sized.
Due to them being medium, they should behave like on web, meaning they should use popover modal, instead of bottom docked modal.
Adding property fromSidebarMediumScreen with proper value, I have resolved the issue, so that on the screens I mentioned, bottom-docked modal is not used, but a proper, popover modal is used in the problematic locations.

Simply adding property fromSidebarMediumScreen resolved the issue, since before
props.fromSidebarMediumScreen ? CONST.MODAL.MODAL_TYPE.POPOVER : CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED (this is taken from the code), it would fallback to bottom-docked modal, when this property wasn't set.

parasharrajat

There are many lint errors. Please fix those. Are you following the contribution guidelines? Please run npm run lint before pushing chnages.

I think the simplest way to do it would be to modify the Popover Component so that we do

 <Modal
            type={!props.isSidebarScreenWidth ? CONST.MODAL.MODAL_TYPE.POPOVER : CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED}

instead of passing fromSidebarMediumScreen which is basically doing the same.

Also, this prop is not defined in the prop definitions. If there is a reason to keep it, please add prop-types for it in every component it is passed as a prop.

parasharrajat · 2022-03-04T15:59:03Z

src/components/AddPaymentMethodMenu.js

@@ -42,6 +42,7 @@ const AddPaymentMethodMenu = props => (
        isVisible={props.isVisible}
        onClose={props.onClose}
        anchorPosition={props.anchorPosition}
+        fromSidebarMediumScreen={!props.isSmallScreenWidth}


Why do you need to pass this when you have already added to BasePopoverMenu

You are right. This will be removed.

parasharrajat · 2022-03-04T16:01:30Z

src/components/AttachmentPicker/index.native.js

                    onModalHide={this.onModalHide}
+                    anchorPosition={this.props.anchorPosition}
+                    fromSidebarMediumScreen={!this.props.isSmallScreenWidth}


How is this prop related to this menu? This menu opens from the composer ➕ .

The menu for attachment picker opens from the same position as the one from the composer ➕.

parasharrajat · 2022-03-04T16:02:52Z

src/styles/styles.js

+    createPaymentMethodMenuContainer: {
+        top: 380,
+    },


I don't see why do we need to pass the width now. If it was working fine on web without it.

This is because let position = getClickedElementLocation(nativeEvent); has native implementation which returns just { bottom: 0, left: 0 }

Can't we use https://reactnative.dev/docs/0.64/pressevent to calculate the values?

@parasharrajat Thanks for the docs.
Looking at the example:

{ changedTouches: [PressEvent], identifier: 1, locationX: 8, locationY: 4.5, pageX: 24, pageY: 49.5, target: 1127, timestamp: 85131876.58868201, touches: [] }

we could theoretically calculate the top left coordinate of the pressed component, by using pageX, pageY and locationX, locationY. We would still need to add a height value of the pressed component to y coordinate, to move the popover to its bottom.
What do you think about that?

Ok. I think that's fine if height is needed let's calculate it. But I don't think that is needed. Setting the top and left edges should be fine. the menu will always be rendered top-down so we should be good without height.

srdjanrist · 2022-03-07T11:43:52Z

There are many lint errors. Please fix those. Are you following the contribution guidelines? Please run npm run lint before pushing chnages.

I think the simplest way to do it would be to modify the Popover Component so that we do
 <Modal
            type={!props.isSidebarScreenWidth ? CONST.MODAL.MODAL_TYPE.POPOVER : CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED}
instead of passing fromSidebarMediumScreen which is basically doing the same.

Also, this prop is not defined in the prop definitions. If there is a reason to keep it, please add prop-types for it in every component it is passed as a prop.

Yes. This could be done that way. Would you like this prop isSidebarScreenWidth to be passed from outer component, or should I wrap Popover with withWindowDimensions, so it has properties from window dimensions?

parasharrajat · 2022-03-07T12:09:34Z

Wrapping with withWindowDimensions is fine.

parasharrajat · 2022-03-17T15:58:48Z

src/components/Popover/index.native.js

@@ -3,7 +3,7 @@ import React from 'react';
 import {propTypes as popoverPropTypes, defaultProps} from './popoverPropTypes';
 import CONST from '../../CONST';
 import Modal from '../Modal';
-import {windowDimensionsPropTypes} from '../withWindowDimensions';
+import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';

 const propTypes = {
    ...(_.omit(popoverPropTypes, _.keys(windowDimensionsPropTypes))),


Needs correction now.

Updated. I have left only that propTypes from popoverPropTypes without omitting windowDimensionsProps

parasharrajat · 2022-03-17T16:00:19Z

src/libs/getClickedElementLocation/index.native.js

    return {
-        bottom: 0,
-        left: 0,
+        bottom: nativeEvent.nativeEvent.pageY - nativeEvent.nativeEvent.locationY,


nativeEvent.nativeEvent looks confusing.

Agree. Based on previous handling of this, I have changed the code that calls this method to pass the nested nativeEvent property instead of the whole object.

parasharrajat · 2022-03-17T16:03:59Z

src/components/AvatarWithImagePicker.js

@@ -186,7 +186,10 @@ class AvatarWithImagePicker extends React.Component {
                            : (
                                <DefaultAvatar />
                            )}
-                        <AttachmentPicker type={CONST.ATTACHMENT_PICKER_TYPE.IMAGE}>
+                        <AttachmentPicker
+                            anchorPosition={styles.createMenuPositionProfile}


Suggested change

anchorPosition={styles.createMenuPositionProfile}

anchorPosition={this.props.anchorPosition}

parasharrajat · 2022-03-17T16:08:01Z

src/components/AttachmentPicker/index.native.js

                    onModalHide={this.onModalHide}
+                    anchorPosition={this.props.anchorPosition}


Did you add the new prop definition?

I have updated the prop definiton.

srdjanrist · 2022-03-28T16:16:55Z

@parasharrajat
Thanks for the videos.
I might have misunderstood something then.
The fadein-fadeout animation is only on native platforms.

Second video shows Android, and the animation is present. Is it not supposed to be there?

parasharrajat · 2022-03-28T16:19:01Z

The motive is to keep the same animation on all platforms. When Menu opens from bottom on Mobile native, the animation is differnt. But when they behave like a web on tablets. They should have same animations.

cc: @Expensify/design

shawnborton · 2022-03-28T18:58:59Z

I agree with that Rajat.

srdjanrist · 2022-03-30T10:45:43Z

Thanks for the input!
I have updated the pull request with a new commit that handles animations on smaller screen sizes on web.
My tests have went over few devices with various sizes, and I couldn't find any inconsistencies now.
Please, double check this @parasharrajat, so that we can proceed.

parasharrajat · 2022-03-30T15:13:55Z

src/components/Modal/BaseModal.js

+                animationInTiming={this.props.disableAnimation ? 1 : this.props.animationInTiming}
+                animationOutTiming={this.props.disableAnimation ? 1 : this.props.animationOutTiming}


Not happy with this change. Can't we just follow the same pattern as we do on the web?

Updated the pull request with a recommended change. 😊

parasharrajat

The code looks fine. I will test it shortly.

parasharrajat · 2022-04-04T15:47:47Z

Testing it. Finally fixed my Mac.

parasharrajat · 2022-04-05T19:34:17Z

Didn't complete my testing. I will finish it up tomorrow.

parasharrajat

Issue on Android Tablet.

Steps:

Open the payments page.
Click at the middle of any payment method.
Menu is off the screen.

Also pleas merge main.

srdjanrist · 2022-04-10T15:17:54Z

Comment

> Issue on Android Tablet. ![image](https://user-images.githubusercontent.com/24370807/162441455-63bf6f7a-6056-45f8-8673-d5f55ded6da6.png) > > Steps: > > 1. Open the payments page. > 2. Click at the middle of any payment method. > 3. Menu is off the screen. > > Also pleas merge main.

I have looked into this.
What I noticed is that depeneding on which part of the menu item is clicked, parameters from onPress callback are different. Actually, besides locationX and locationY, also target is different.

Touching the "Add payment method" part yields:
"locationX": 4.11676025390625, "locationY": 9.80316162109375, "target": 27237

Touching anywhere beside the "Add payment method" part yields:
"locationX": 351.5091552734375, "locationY": 11.81427001953125, "target": 27247
As you can see the target property is different as well.

By using this location and on-screen location, the popover's position is calculated.
Due to positions being different (depending on where the menu item is clicked), I have no way to precisely and consistently calculate the position of popover.

Have you had this situation before? Do you have any suggestions for improvement?

Julesssss · 2022-04-11T09:48:08Z

Hey @srdjanrist, we'll need to resolve the issue @parasharrajat mentioned, as it doesn't look ideal. I'm afraid I don't have any suggestions for you, but sharing the issue in the shared Slack channel might be a good idea. As it's quite possible that someone has faced and solved this problem before.

srdjanrist · 2022-04-18T12:11:09Z

I have consulted people on Slack, and will try their approach as well.

I was thinking of another approach to this.
@parasharrajat Would it be fine to update MenuItem component, refactor a little bit, and make a transparent View, positioned over the whole menu item. That way, this new overlay view would take all touch events and positioning would be always relative to the whole menu item, instead of each separate child view individually?

parasharrajat · 2022-04-18T14:38:36Z

That would be Overkill. Can't we use measureInWindow to get the coordinates?

srdjanrist · 2022-04-21T09:25:33Z

That would be Overkill. Can't we use measureInWindow to get the coordinates?

I will check this today.

srdjanrist · 2022-04-25T23:00:22Z

That would be Overkill. Can't we use measureInWindow to get the coordinates?

I will check this today.

Updated MenuItem with code for measuring layout with the mentioned method

parasharrajat · 2022-04-26T10:30:58Z

src/components/MenuItem.js

-const MenuItem = props => (
-    <Pressable
-        onPress={(e) => {


This change is not acceptable. We don't have to touch the MenuItem for this. Instead, use the ref to calculate whatever is needed on the parent components that use it.

Hi @srdjanrist, could you please address this comment so we can move forward :)

github-actions · 2022-07-13T09:08:10Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

0 out of 2 committers have signed the CLA.
❌ @srdjan Ristanovic
❌ @srdjanrist
Srdjan Ristanovic, srdjanrist seem not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request}

Srdjan Ristanovic added 4 commits March 2, 2022 18:49

Update BasePopoverMenu to include check for small screen size

5ba0ced

Update AttachmentPicker to check for screen width on native

fdad9d6

Update AddPaymentMethodMenu to properly show highlight on payment sel…

9555227

…ection depending on screen size

Update BasePaymentsPage to position payment method menu on nonsmall n…

ec1782c

…ative devices

srdjanrist requested a review from a team as a code owner March 2, 2022 18:02

MelvinBot requested review from Julesssss and parasharrajat and removed request for a team March 2, 2022 18:02

Srdjan Ristanovic added 2 commits March 2, 2022 23:55

Merge main to feature branch

386c9ce

Fix linting issues

43a065d

parasharrajat suggested changes Mar 3, 2022

View reviewed changes

parasharrajat suggested changes Mar 4, 2022

View reviewed changes

Update component, remove fromSidebarMediumScreen

2b72247

Srdjan Ristanovic added 8 commits March 16, 2022 22:02

Update method for calculating click position on native

4485d7c

Remove unused code

2c04b2b

Merge main branch

7aaf507

Update Popover component, add withWindowDimension

ba97201

Remove usage of fromSidebarMediumScreen prop in components

b79bcd0

Resolve lint issues

2347155

Remove needless new lines

75b9383

Merge main branch

0cdfef3

parasharrajat suggested changes Mar 17, 2022

View reviewed changes

Update and refactor code parts

01c25e1

Update how animations are handled on small devices

53088a4

Merge branch 'Expensify:main' into popover-fixes-on-tablet

03eaeff

parasharrajat suggested changes Mar 30, 2022

View reviewed changes

Srdjan Ristanovic added 2 commits March 31, 2022 12:25

Update animations on different platforms

dcaefb5

Merge branch 'Expensify:main' into popover-fixes-on-tablet

e844694

parasharrajat reviewed Apr 1, 2022

View reviewed changes

parasharrajat suggested changes Apr 8, 2022

View reviewed changes

Merge main branch

1274f09

thesahindia mentioned this pull request Apr 12, 2022

present CTAs as green buttons #8609

Merged

87 tasks

Srdjan Ristanovic added 3 commits April 21, 2022 23:43

Merge main branch

0e8c648

Update MenuItem to handle absolute position from measureInWindow

0573370

Merge main branch

bbcaa8f

parasharrajat suggested changes Apr 26, 2022

View reviewed changes

Julesssss self-assigned this May 9, 2022

thesahindia mentioned this pull request May 10, 2022

Payments -Two options are hidden when you click on > button #8922

Closed

NikkiWines mentioned this pull request May 10, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

Julesssss closed this Jul 13, 2022

	anchorPosition={styles.createMenuPositionProfile}
	anchorPosition={this.props.anchorPosition}

		onModalHide={this.onModalHide}
		anchorPosition={this.props.anchorPosition}

		animationInTiming={this.props.disableAnimation ? 1 : this.props.animationInTiming}
		animationOutTiming={this.props.disableAnimation ? 1 : this.props.animationOutTiming}

Popover fixes on tablet #7985

Popover fixes on tablet #7985

Conversation

srdjanrist commented Mar 2, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Tested On

Screenshots

parasharrajat left a comment

Choose a reason for hiding this comment

srdjanrist commented Mar 3, 2022

parasharrajat commented Mar 3, 2022 • edited Loading

srdjanrist commented Mar 3, 2022

parasharrajat commented Mar 3, 2022 • edited Loading

srdjanrist commented Mar 4, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

srdjanrist commented Mar 7, 2022

parasharrajat commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdjanrist commented Mar 28, 2022

parasharrajat commented Mar 28, 2022

shawnborton commented Mar 28, 2022

srdjanrist commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat commented Apr 4, 2022

parasharrajat commented Apr 5, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

srdjanrist commented Apr 10, 2022

Julesssss commented Apr 11, 2022

srdjanrist commented Apr 18, 2022

parasharrajat commented Apr 18, 2022

srdjanrist commented Apr 21, 2022

srdjanrist commented Apr 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2022

srdjanrist commented Mar 2, 2022 •

edited

Loading

parasharrajat commented Mar 3, 2022 •

edited

Loading

parasharrajat commented Mar 3, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat Mar 13, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading