refactor CreateMenu to have dynamic items #1548

Maftalion · 2021-02-23T06:53:09Z

Details

updated CreateMenu to take a dynamic list of items based on MENU_ITEM_KEYS enum
updated ReportActionCompose to open the original attachment modal when AttachmentPicker is selected

Fixed Issues

Fixes https://github.com//issues/1508

Tests

log in and click on the + fab to verify list has new items
enter a convo with more than one person to see SplitBill|AddAttachment options when clicking the attach icon
enter a convo with one person to see RequestMoney |AddAttachment options when clicking the attach icon

Tested On

Video Walkthrough

Screen.Recording.2021-02-22.at.10.33.44.PM.mov

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2021-02-23T06:53:25Z

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

Maftalion · 2021-02-23T06:55:23Z

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

Maftalion · 2021-02-23T06:57:34Z

src/pages/home/report/ReportActionCompose.js

+                                                style={styles.chatItemAttachButton}
+                                                underlayColor={themeColors.componentBG}
+                                            >
+                                                <Icon src={Paperclip} />


left this icon as a Paperclip, might want to update to make it seem more like a menu icon

Ah good point, I forgot to mention but we have a new icon for this.

src/pages/home/report/ReportActionCompose.js

thienlnam

Thanks for the PR! Code looks mostly good, just a couple of comments

src/components/CreateMenu.js

src/pages/home/report/ReportActionCompose.js

thienlnam · 2021-02-23T23:03:25Z

Also, could you update your PR to fix the merge conflict and the description to update the fixed issued link, and I think you mean

enter a convo with more one person to see RequestMoney |AddAttachment options when clicking the attach icon

Enter a convo with just one person?

…te-menu # Conflicts: # src/pages/home/sidebar/SidebarScreen.js

src/components/CreateMenu.js

src/pages/home/report/ReportActionCompose.js

Julesssss

Looking good 👍

Apart from a few minor comments, I think the main change necessary is an improvement to the hack that @thienlnam pointed out.

thienlnam · 2021-02-24T23:38:26Z

Tried to test the PR but it doesn't seem like the Add Attachment button is working for me at all even with the timeout

Maftalion · 2021-02-25T00:35:16Z

Tried to test the PR but it doesn't seem like the Add Attachment button is working for me at all even with the timeout

hmm looks the change to conditionally render the component with {this.state.isMenuVisible ? ( causes it to not work. going to investigate further.

Maftalion · 2021-02-25T02:40:11Z

@Julesssss looks like conditionally rendering using this.state.isMenuVisible causes the component to unmount before it can call onModalHide.
I can fix this by adding:

    componentWillUnmount() {
        this.onModalHide();
    }

let me know if this is fine, otherwise I can revert the conditional rendering.

I haven't been able to get the modals to work without using setTimeout, what's strange is onModalHide is supposed to be the solution to opening a modal once another closes so not sure why it's not working.

Julesssss · 2021-02-25T15:25:14Z

@Maftalion ah, lets revert to the previous solution then.

I'm also not sure why the callback doesn't work without setTimeout, it might be worth sharing the issue in the Contributor chat slack channel to see if anyone else has an idea.

Julesssss · 2021-02-26T13:04:45Z

Hey @Expensify/design, would you mind sharing the new plus icon asset that will replace the paperclip icon. Shown in the following screenshot:

Julesssss · 2021-02-26T15:56:21Z

Hmm, the setTimeout issue is a tricky one.

I tried looking for a specific callback that was closing the AttachmentPicker with no luck — the ReportActionCompose Component props aren’t changing (I thought this might be the issue)

I noticed that only iOS has the Attachment closing issue btw. Android, Web & Desktop all work fine, so it might be worth looking for similar iOS bugs/solutions.

I’m a bit confused by the reverse nesting of within and wonder if this is necessary/could be made simpler? This isn't in scope for this PR though

Maftalion · 2021-02-27T08:27:36Z

I’m a bit confused by the reverse nesting of within and wonder if this is necessary/could be made simpler? This isn't in scope for this PR though - yeah, definitely makes it tricky to debug as well

Regarding the plus asset, I noticed we already have it so I can just swap it in. Do you also want the border gone like your screenshot above or is this fine?

shawnborton · 2021-02-27T10:05:03Z

@Maftalion your screenshot looks perfect, let's go with that.

Julesssss · 2021-03-02T15:40:46Z

Hey @Maftalion. just wanted to check in and provide some guidance for moving forward.

As we'll need to resolve the issue without the setTimeout workaround, the next step will be to locate the cause (bug in Expensify.cash OR bug in external library). If this is an internal bug we should aim to fix it as part of the PR, but in the case where this is an external issue we could leave the PR on hold and mark the UpWork complete.

We would then aim to resolve the issue within the offending library and if this was of interest to you we would treat this as a separate issue and provide an additional payment. Once the library issue is fixed, this PR can be taken off hold and merged.

# Conflicts: # src/components/Icon/Expensicons.js

Maftalion · 2021-03-03T07:29:02Z

Hey @Julesssss, I've been unable to get it to work without the setTimeout.

I've tried looking for re-render issues, moving things around and calling the Api directly instead of openPicker but all attempts were unsuccessful.

I then looked into the react-native-image-picker and there seems to be major breaking changes between the current version and the latest so the whole actionsheet would need to be redone in order to test the latest version. But i also tested swapping it out with RN's built in ActionSheetIOS and that led to the same behavior.

Which meant the issue was probably with the modal library react-native-modal. There seems to be a lot of similar issues as this: https://github.com/react-native-modal/react-native-modal/issues?q=is%3Aissue+onmodalhide and a few lead to the same solution of just using setTimeout.

Julesssss · 2021-03-08T15:52:33Z

Hey @Maftalion, apologies for the delayed response. Thanks for looking into the modal library and potential solutions.

Lets leave this PR on hold while I try to get the react-native-modal library issue created. I'll update this PR once I have an update.

Julesssss · 2021-03-19T14:51:43Z

Hi @Maftalion. I'm sorry for keeping you waiting for so long!

To move forward, we'll merge this PR with the setTimeout workaround.

Would you mind fixing the conflicts with master to finish up here? I'll make sure to add a bonus to the payout as this is due to my delay.

# Conflicts: # src/CONST.js # src/components/CreateMenu.js # src/components/Icon/Expensicons.js # src/pages/home/ReportScreen.js # src/pages/home/report/ReportActionCompose.js # src/pages/home/report/ReportView.js # src/pages/home/sidebar/SidebarScreen.js

Maftalion · 2021-03-19T16:37:14Z

@Julesssss no worries! fixed the merge conflicts

src/pages/home/report/ReportActionCompose.js

Julesssss · 2021-03-19T18:55:58Z

Okay, we're almost there.

I'm going to branch from here to prepare a PR locking the feature behind the new internal IOU Beta flag, once this is completed I will merge this issue and quickly follow it up by merging the beta flag PR.

Should be first thing Monday.

Out of date

isagoico · 2021-03-25T21:17:31Z

Hello! @Julesssss @Maftalion
We were about to do the QA for this PR and came across this other PR #1548. On the latter the options that we need to test for this one are actually expected to be hidden right?
This would block us from being to execute the steps for this PR.

Julesssss · 2021-03-26T09:40:39Z

Hi @isagoico.

Yeah, good point. Let's skip testing for this one in that case. I'll add the test steps to the PR which re-enables the new options.

refactor CreateMenu to have dynamic items

4ab6780

Maftalion requested a review from a team as a code owner February 23, 2021 06:53

botify requested review from thienlnam and removed request for a team February 23, 2021 06:53

Maftalion commented Feb 23, 2021

View reviewed changes

src/pages/home/report/ReportActionCompose.js Show resolved Hide resolved

thienlnam previously requested changes Feb 23, 2021

View reviewed changes

src/components/CreateMenu.js Outdated Show resolved Hide resolved

src/pages/home/report/ReportActionCompose.js Show resolved Hide resolved

Maftalion added 3 commits February 23, 2021 18:56

refactor menu items into CONST

a73e58d

Merge remote-tracking branch 'upstream/master' into matt-dynamic-crea…

27d53eb

…te-menu # Conflicts: # src/pages/home/sidebar/SidebarScreen.js

add missing import

ff2c70d

Julesssss reviewed Feb 24, 2021

View reviewed changes

src/components/CreateMenu.js Show resolved Hide resolved

Julesssss reviewed Feb 24, 2021

View reviewed changes

src/components/CreateMenu.js Show resolved Hide resolved

Julesssss reviewed Feb 24, 2021

View reviewed changes

src/pages/home/report/ReportActionCompose.js Show resolved Hide resolved

Julesssss previously approved these changes Feb 24, 2021

View reviewed changes

update menu visibility logic

728bca0

Maftalion dismissed Julesssss’s stale review via 728bca0 February 24, 2021 16:05

revert conditional rendering of menu

8d4ecfa

update to use plus icon

c288639

Maftalion added 2 commits March 2, 2021 09:05

Merge branch 'master' into matt-dynamic-create-menu

b3ac7d9

Merge branch 'master' into matt-dynamic-create-menu

f06d865

# Conflicts: # src/components/Icon/Expensicons.js

Julesssss assigned Maftalion Mar 8, 2021

Julesssss mentioned this pull request Mar 19, 2021

iOS issue: Modal closes immediately after launch #1913

Closed

Maftalion added 2 commits March 19, 2021 09:07

fix merge conflicts

3fc4b04

Julesssss reviewed Mar 19, 2021

View reviewed changes

src/pages/home/report/ReportActionCompose.js Outdated Show resolved Hide resolved

reduce time, fix proptype

801f92a

Julesssss approved these changes Mar 19, 2021

View reviewed changes

Julesssss requested a review from thienlnam March 22, 2021 15:18

Julesssss merged commit fe0ea56 into Expensify:master Mar 22, 2021

github-actions bot locked and limited conversation to collaborators Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor CreateMenu to have dynamic items #1548

refactor CreateMenu to have dynamic items #1548

Maftalion commented Feb 23, 2021 •

edited

Loading

github-actions bot commented Feb 23, 2021 •

edited

Loading

Maftalion commented Feb 23, 2021

Maftalion Feb 23, 2021

Julesssss Feb 26, 2021

thienlnam left a comment

thienlnam commented Feb 23, 2021

Julesssss left a comment

thienlnam commented Feb 24, 2021

Maftalion commented Feb 25, 2021

Maftalion commented Feb 25, 2021

Julesssss commented Feb 25, 2021

Julesssss commented Feb 26, 2021

Julesssss commented Feb 26, 2021 •

edited

Loading

Maftalion commented Feb 27, 2021

shawnborton commented Feb 27, 2021

Julesssss commented Mar 2, 2021

Maftalion commented Mar 3, 2021

Julesssss commented Mar 8, 2021

Julesssss commented Mar 19, 2021

Maftalion commented Mar 19, 2021

Julesssss commented Mar 19, 2021

isagoico commented Mar 25, 2021

Julesssss commented Mar 26, 2021

refactor CreateMenu to have dynamic items #1548

refactor CreateMenu to have dynamic items #1548

Conversation

Maftalion commented Feb 23, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Video Walkthrough

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Feb 23, 2021 • edited Loading

Maftalion commented Feb 23, 2021

Maftalion Feb 23, 2021

Choose a reason for hiding this comment

Julesssss Feb 26, 2021

Choose a reason for hiding this comment

thienlnam left a comment

Choose a reason for hiding this comment

thienlnam commented Feb 23, 2021

Julesssss left a comment

Choose a reason for hiding this comment

thienlnam commented Feb 24, 2021

Maftalion commented Feb 25, 2021

Maftalion commented Feb 25, 2021

Julesssss commented Feb 25, 2021

Julesssss commented Feb 26, 2021

Julesssss commented Feb 26, 2021 • edited Loading

Maftalion commented Feb 27, 2021

shawnborton commented Feb 27, 2021

Julesssss commented Mar 2, 2021

Maftalion commented Mar 3, 2021

Julesssss commented Mar 8, 2021

Julesssss commented Mar 19, 2021

Maftalion commented Mar 19, 2021

Julesssss commented Mar 19, 2021

isagoico commented Mar 25, 2021

Julesssss commented Mar 26, 2021

Maftalion commented Feb 23, 2021 •

edited

Loading

github-actions bot commented Feb 23, 2021 •

edited

Loading

Julesssss commented Feb 26, 2021 •

edited

Loading