Display create menu within the chat area and add bottom up transition. #2222

npsedhain · 2021-04-03T17:43:03Z

Details

CreateMenu opens within the chat area.
The animation is changed to bottom-up transition.

Fixed Issues

Fixes [GH_LINK_2090](https://github.com//issues/2090)

Tests

Enter a chat window.
Click on the plus icon next to the chat input.
The component opens within the chat area and not outside it.
The transition is changed to bottom-up from left to right.

Tested On

Web
Mobile Web
Desktop

Screenshots

Web

Screen.Recording.2021-04-03.at.11.31.35.AM.mov

Mobile Web

(Doesn't break anything on a smaller screen)

Screen.Recording.2021-04-03.at.11.25.04.PM.mov

Desktop

Screen.Recording.2021-04-03.at.11.33.16.AM.mov

tgolen

I guess this is OK when there are multiple lines in the input field?

cc @shawnborton

tgolen · 2021-04-06T22:28:52Z

src/components/CreateMenu.js

@@ -51,6 +79,7 @@ class CreateMenu extends PureComponent {
    }

    render() {
+        console.debug(this.props.popOverType, this.getAnchorPosition());


I think this can be removed? I don't think it's very useful to have in production.

Sorry for this one, will remove it.

tgolen · 2021-04-06T22:30:16Z

src/components/CreateMenu.js

@@ -59,7 +88,8 @@ class CreateMenu extends PureComponent {
                    this.onModalHide();
                    this.resetOnModalHide();
                }}
-                anchorPosition={styles.createMenuPosition}
+                anchorPosition={this.getAnchorPosition(this.props.popOverType)}


If the anchor position is determined by the popover type, and the popover type is already passed as a prop, then I think it would be cleaner to have anchor position determined inside of the <Popover> component. Would this be possible?

tgolen · 2021-04-06T22:31:09Z

src/components/CreateMenu.js

@@ -84,5 +114,6 @@ class CreateMenu extends PureComponent {
 }

 CreateMenu.propTypes = propTypes;
+CreateMenu.defaultProps = defaultProps;
 CreateMenu.displayName = 'CreateMenu';


I know class components don't need displayName, but I'm not sure about PureComponents. Can the display name be removed?

Do you know?

tgolen · 2021-04-06T22:31:32Z

src/components/CreateMenu.js

+const defaultProps = {
+    popOverType: CONST.MODAL.MODAL_TYPE.POPOVER_LEFT_DOCKED,
+};
+
 class CreateMenu extends PureComponent {


Is there a reason this is a PureComponent instead of just a normal Component?

The pure component was implemented beforehand, not sure why it was done though.

OK, I'll check on that and dig a little deeper. No need to let this block your PR since you didn't add it.

tgolen · 2021-04-06T22:32:32Z

src/components/Modal/ModalPropTypes.js

@@ -23,7 +23,9 @@ const propTypes = {
        CONST.MODAL.MODAL_TYPE.CONFIRM,
        CONST.MODAL.MODAL_TYPE.CENTERED,
        CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED,
-        CONST.MODAL.MODAL_TYPE.POPOVER,
+        CONST.MODAL.MODAL_TYPE.POPOVER_CENTER_BOTTOM,


Maybe it would be easier to do something like type: PropTypes.oneOf(_.values(CONST.MODAL.MODAL_TYPE))?

tgolen · 2021-04-06T22:33:02Z

src/components/Popover/PopoverPropTypes.js

@@ -12,6 +12,8 @@ const propTypes = {
        bottom: PropTypes.number,
        left: PropTypes.number,
    }).isRequired,
+
+    popOverType: modalPropTypes.type,


This is missing an inline comment to explain it.

shawnborton · 2021-04-06T22:38:49Z

I think it's okay that the menu location doesn't change even if the textbox has multiple lines and grows taller. But I think the location of the menu feels a little bit too high above the text input right now. Could we place it like 8 or 16px above the top edge of a one-line chat input?

trjExpensify · 2021-04-12T11:34:10Z

Hey @npsedhain, can we get an update on the changes requested here please? Thanks!

npsedhain · 2021-04-12T11:36:30Z

Hey @trjExpensify, sorry for the delay, will push the changes within an hour!

…menu

npsedhain · 2021-04-12T12:24:58Z

I have pushed the recommended changes, do let me know if there is more!

cc: @tgolen @trjExpensify

tgolen · 2021-04-12T21:39:20Z

Looks like there is a conflict.

…menu

npsedhain · 2021-04-13T12:07:36Z

Hey @tgolen, I have resolved the conflicts.

tgolen · 2021-04-13T16:25:08Z

Hm, looks like there are still conflicts. Maybe you didn't push your changes or there are just more conflicts?

npsedhain · 2021-04-13T16:46:11Z

Aah I did push it, maybe because something else was merged, will fix it in a minute.

…menu

npsedhain · 2021-04-13T17:23:16Z

@tgolen Its fixed!

tgolen

Sorry for the delay on this!

tgolen · 2021-04-15T17:42:08Z

src/components/CreateMenu.js

@@ -84,5 +114,6 @@ class CreateMenu extends PureComponent {
 }

 CreateMenu.propTypes = propTypes;
+CreateMenu.defaultProps = defaultProps;
 CreateMenu.displayName = 'CreateMenu';


Do you know?

roryabraham · 2021-04-15T19:20:59Z

This PR caused a regression. Normally I would just revert it, but unfortunately the base branch was renamed soon after this was merged, so the automatic revert is not an option. @npsedhain Can you create a pull request to fix this regression?

github-actions · 2021-04-15T19:21:15Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

roryabraham · 2021-04-15T20:06:36Z

Since this will be a deploy blocker, I am creating a PR here to fix the regression.

OSBotify · 2021-04-19T22:41:56Z

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham · 2021-04-21T18:25:54Z

Just noticed that this other PR actually split up the implementation of CreateMenu a bit and deleted CreateMenu.js, and this one had a bad merge which did not include those changes. So now we have two divergent CreateMenu modules 😬

Display create menu within the chat area and add bottom up transition.

9d74c30

npsedhain requested a review from a team as a code owner April 3, 2021 17:43

MelvinBot requested review from tgolen and removed request for a team April 3, 2021 17:43

tgolen requested changes Apr 6, 2021

View reviewed changes

npsedhain added 2 commits April 12, 2021 18:04

Merge remote-tracking branch 'upstream/master' into npsedhain-create-…

1068005

…menu

Refactor: Resolve the comments, removed anchor position and others.

ee27ea5

tgolen previously approved these changes Apr 12, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into npsedhain-create-…

0be1c12

…menu

Merge remote-tracking branch 'upstream/master' into npsedhain-create-…

4bcd10f

…menu

npsedhain dismissed tgolen’s stale review via 4bcd10f April 13, 2021 16:50

tgolen approved these changes Apr 15, 2021

View reviewed changes

tgolen merged commit e7330c4 into Expensify:master Apr 15, 2021

OSBotify mentioned this pull request Apr 15, 2021

Deploy Checklist: Expensify.cash 2021-04-13 #2380

Closed

66 tasks

roryabraham mentioned this pull request Apr 15, 2021

Fix popovers after regression #2424

Merged

5 tasks

roryabraham mentioned this pull request Apr 21, 2021

Fix CreateMenu and mobile safari attachment picker #2516

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display create menu within the chat area and add bottom up transition. #2222

Display create menu within the chat area and add bottom up transition. #2222

npsedhain commented Apr 3, 2021

tgolen left a comment

tgolen Apr 6, 2021

npsedhain Apr 7, 2021

tgolen Apr 6, 2021

tgolen Apr 6, 2021

tgolen Apr 15, 2021

tgolen Apr 6, 2021

npsedhain Apr 8, 2021

tgolen Apr 8, 2021

tgolen Apr 6, 2021

tgolen Apr 6, 2021

shawnborton commented Apr 6, 2021

trjExpensify commented Apr 12, 2021

npsedhain commented Apr 12, 2021

npsedhain commented Apr 12, 2021

tgolen commented Apr 12, 2021

npsedhain commented Apr 13, 2021

tgolen commented Apr 13, 2021

npsedhain commented Apr 13, 2021

npsedhain commented Apr 13, 2021

tgolen left a comment

tgolen Apr 15, 2021

roryabraham commented Apr 15, 2021

github-actions bot commented Apr 15, 2021 •

edited

Loading

roryabraham commented Apr 15, 2021 •

edited

Loading

OSBotify commented Apr 19, 2021

roryabraham commented Apr 21, 2021

Display create menu within the chat area and add bottom up transition. #2222

Display create menu within the chat area and add bottom up transition. #2222

Conversation

npsedhain commented Apr 3, 2021

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Mobile Web

Desktop

tgolen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnborton commented Apr 6, 2021

trjExpensify commented Apr 12, 2021

npsedhain commented Apr 12, 2021

npsedhain commented Apr 12, 2021

tgolen commented Apr 12, 2021

npsedhain commented Apr 13, 2021

tgolen commented Apr 13, 2021

npsedhain commented Apr 13, 2021

npsedhain commented Apr 13, 2021

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented Apr 15, 2021

github-actions bot commented Apr 15, 2021 • edited Loading

roryabraham commented Apr 15, 2021 • edited Loading

OSBotify commented Apr 19, 2021

roryabraham commented Apr 21, 2021

github-actions bot commented Apr 15, 2021 •

edited

Loading

roryabraham commented Apr 15, 2021 •

edited

Loading