shawnborton commented 5 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Issue reported by: @shawnborton Slack conversation: Link [Internal]

Feature Request

We want to update some of the icons found in our global create menu as well as the money request flows: CleanShot 2024-04-15 at 13 09 22@2x

Notes:

For Track expense, we want to use the coins icon
For Request money, we want to use the receipt icon
For Send money, we want to use the money/cash icon
For Scan in the money request flows, we want to use the scan receipt icon
The updates made in the global create menu and segmented control should also be reflected in the Quick action row found in global create

Note that all of these icons already exist in the App repo, we just need to use them in the places mentioned above.

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Note four our internal team: given that this is just very small file name changes, we should drop the bounty down to $125.

cc @Expensify/design @kevinksullivan @mountiny @quinthar

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0162b090372a61cda0
Upwork Job ID: 1779768720839634944
Last Price Increase: 2024-04-15
Automatic offers:

tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @puneetlath

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 5 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0162b090372a61cda0

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

ghost commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Update icons in global create and money request flows

What is the root cause of that problem?

Feature Request

What changes do you think we should make in order to solve the problem?

Adding new icons for :

For Track expense, we want to use the coins icon
For Request money, we want to use the receipt icon
For Send Money, we want to use the money/cash icon
For Scan in the money request flows, we want to use the scan receipt icon The updates made in the global create menu and segmented control should also be reflected in the Quick action row found in global create as mentioned in the comment We can find these icons in the assets add it to their respective components or places that we need to add for example - For Track expense, we want to use the coins icon in the global create flow and money request flow.
What alternative solutions did you explore? (Optional)

N/A

ZhenjaHorbach commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Update icons in global create and money request flows

What is the root cause of that problem?

New features

What changes do you think we should make in order to solve the problem?

To fix this issue we need update icons in global create and money request flows following instructions

What alternative solutions did you explore? (Optional)

NA

ghost commented 5 months ago

updated proposal with changes to be added.

allgandalf commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Update icons in global create and money request flows

What is the root cause of that problem?

Feature Request

What changes do you think we should make in order to solve the problem?

We need to update icons from the global floating menu below:

https://github.com/Expensify/App/blob/b5fdc877accaa7aea216291c249051acb481ad08/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx#L256-L264

The icons can be found in the expensicons component: https://github.com/Expensify/App/blob/b5fdc877accaa7aea216291c249051acb481ad08/src/components/Icon/Expensicons.ts#L1

What alternative solutions did you explore? (Optional)

ShridharGoel commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

We need to use new icons in global create and money request flows.

What is the root cause of that problem?

New update.

What changes do you think we should make in order to solve the problem?

Update the icons in menu items:

https://github.com/Expensify/App/blob/b5fdc877accaa7aea216291c249051acb481ad08/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx#L262-L329

Update the icons in quick actions as well:

https://github.com/Expensify/App/blob/b5fdc877accaa7aea216291c249051acb481ad08/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx#L72-L91

Update the tab selector icon for scan:

https://github.com/Expensify/App/blob/b5fdc877accaa7aea216291c249051acb481ad08/src/components/TabSelector/TabSelector.tsx#L28-L29

Update the icons below as well:

https://github.com/Expensify/App/blob/2815cf2f6bc418f39cc9b7fa42132efdaee05137/src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.tsx#L124-L146

All new icons are in Expensicons.ts.

shubham1206agra commented 5 months ago

@kevinksullivan @mountiny Just confirming here, shouldn't this request come under https://github.com/Expensify/App/issues/36748?

tienifr commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

We want to replace the icons in Global Create and Money Request flow

What is the root cause of that problem?

This is new feature.

What changes do you think we should make in order to solve the problem?

Since this is a simple change, I'd like to additionally suggest a refactor that will make our life easier in the future. As we can see here, we want to use icons consistently in the app for each feature. For example, "Request money" might appear in various places, and we're using Expensicons.[IconName] for each of them. This leads to a problem, when we need to make a change across the app like this, we have to find every since instance of the feature being mentioned, and replace the Icon by another hardcoded Expensicons.[IconName]. And each place we miss would be a regression.

So what I'd like to propose here is that we add a new method getIconForAction(actionName) where we'd centralize the icon retrieval for each core flow in the app. So when we need to find the icon for track expense, for example, we can just do getIconForAction(CONST.IOU.TYPE.TRACK_EXPENSE). Then later if we want to change icons for a core flow, we can just change inside getIconForAction and it will be reflected through-out the app.

So the steps to fix this issue are:

Define getIconForAction and make sure we handle the IOU actions where we'd like to change icons here, the Icons are already all defined, we just need to return the correct icons for each action, as defined in the OP.
Use the new method to get the icon for the Global create here
Use the new method to get the icon for the Quick actions here
Use the new method to get the icon for the actions in the Composer "Plus" context menu here (every other proposal missed this part, and it was also not mentioned in the OP, but it needs to be updated to be consistent. Note for reviewer: This proposal was updated to include this change after I posted my proposal)
Use the new method to get the icon for the Scan action here

What alternative solutions did you explore? (Optional)

If we're ok with hardcoding the icon for each flow now, we can just replace the icon in each place mentioned above (but still needs to include the 4th step which every other proposals were missing)

shawnborton commented 5 months ago

@shubham1206agra that is a separate issue with a separate problem/solution. This issue does not touch the Split bill icon at all.

trjExpensify commented 5 months ago

Just to confirm after reading the OP, we aren't changing the names of the global create option rows with this issue, right? We're going to wait for the doc Kevin is working on for that, so we do it in tandem with all the other places we reference "request money" / "request" for example?

brkdavis commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Update icons as per clear instructions and wireframes in the description

What is the root cause of that problem?

New feature

For Track expense, we want to use the coins icon
For Submit expense, we want to use the receipt icon
For Pay someone, we want to use the money/cash icon
For Scan in the money request flows, we want to use the scan receipt icon
The updates made in the global create menu and segmented control should also be reflected in the Quick action row found in global create

What changes do you think we should make in order to solve the problem?

As all these are existing icons, no new assets are required. Update references to existing icons from the menu.

Update the icon name in quick actions in FloatingActionButtonAndPopoverOnyxProps.tsx

const getQuickActionIcon = (action: QuickActionName): React.FC<SvgProps> => {
    switch (action) {
        case CONST.QUICK_ACTIONS.REQUEST_MANUAL:
            return Expensicons.MoneyCircle;
        case CONST.QUICK_ACTIONS.REQUEST_SCAN:
            return Expensicons.Receipt;
        case CONST.QUICK_ACTIONS.REQUEST_DISTANCE:
            return Expensicons.Car;
        case CONST.QUICK_ACTIONS.SPLIT_MANUAL:
        case CONST.QUICK_ACTIONS.SPLIT_SCAN:
        case CONST.QUICK_ACTIONS.SPLIT_DISTANCE:
            return Expensicons.Transfer;
        case CONST.QUICK_ACTIONS.SEND_MONEY:
            return Expensicons.Send;
        case CONST.QUICK_ACTIONS.ASSIGN_TASK:
            return Expensicons.Task;
        default:
            return Expensicons.MoneyCircle;
    }
};

Update the icons in the react View that will be rendered.

<View style={styles.flexGrow1}>
            <PopoverMenu
                onClose={hideCreateMenu}
                isVisible={isCreateMenuActive && (!isSmallScreenWidth || isFocused)}
                anchorPosition={styles.createMenuPositionSidebar(windowHeight)}
                onItemSelected={hideCreateMenu}
                fromSidebarMediumScreen={!isSmallScreenWidth}
                menuItems={[
                    {
                        icon: Expensicons.ChatBubble,
                        text: translate('sidebarScreen.fabNewChat'),
                        onSelected: () => interceptAnonymousUser(Report.startNewChat),
                    },
                    ...(canUseTrackExpense
                        ? [
                              {
                                  icon: Expensicons.DocumentPlus,
                                  text: translate('iou.trackExpense'),
                                  onSelected: () =>
                                      interceptAnonymousUser(() =>
                                          IOU.startMoneyRequest(
                                              CONST.IOU.TYPE.TRACK_EXPENSE,
                                              // When starting to create a track expense from the global FAB, we need to retrieve selfDM reportID.
                                              // If it doesn't exist, we generate a random optimistic reportID and use it for all of the routes in the creation flow.
                                              // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
                                              ReportUtils.findSelfDMReportID() || ReportUtils.generateReportID(),
                                          ),
                                      ),
                              },
                          ]
                        : []),
                    {
                        icon: Expensicons.MoneyCircle,
                        text: translate('iou.requestMoney'),
                        onSelected: () =>
                            interceptAnonymousUser(() =>
                                IOU.startMoneyRequest(
                                    CONST.IOU.TYPE.REQUEST,
                                    // When starting to create a money request from the global FAB, there is not an existing report yet. A random optimistic reportID is generated and used
                                    // for all of the routes in the creation flow.
                                    ReportUtils.generateReportID(),
                                ),
                            ),
                    },
                    {
                        icon: Expensicons.Send,
                        text: translate('iou.sendMoney'),
                        onSelected: () =>
                            interceptAnonymousUser(() =>
                                IOU.startMoneyRequest(
                                    CONST.IOU.TYPE.SEND,
                                    // When starting to create a send money request from the global FAB, there is not an existing report yet. A random optimistic reportID is generated and used
                                    // for all of the routes in the creation flow.
                                    ReportUtils.generateReportID(),
                                ),
                            ),
                    },
                    {
                        icon: Expensicons.Task,
                        text: translate('newTaskPage.assignTask'),
                        onSelected: () => interceptAnonymousUser(() => Task.clearOutTaskInfoAndNavigate()),
                    },
                    ...(!isLoading && !Policy.hasActiveChatEnabledPolicies(allPolicies)
                        ? [
                              {
                                  displayInDefaultIconColor: true,
                                  contentFit: 'contain' as ImageContentFit,
                                  icon: Expensicons.NewWorkspace,
                                  iconWidth: 46,
                                  iconHeight: 40,
                                  text: translate('workspace.new.newWorkspace'),
                                  description: translate('workspace.new.getTheExpensifyCardAndMore'),
                                  onSelected: () => interceptAnonymousUser(() => App.createWorkspaceWithPolicyDraftAndNavigateToIt()),
                              },
                          ]
                        : []),
                    ...(quickAction?.action
                        ? [
                              {
                                  icon: getQuickActionIcon(quickAction?.action),
                                  text: quickActionTitle,
                                  label: translate('quickAction.shortcut'),
                                  isLabelHoverable: false,
                                  floatRightAvatars: quickActionAvatars,
                                  floatRightAvatarSize: CONST.AVATAR_SIZE.SMALL,
                                  description: !isEmptyObject(quickActionReport) ? ReportUtils.getReportName(quickActionReport) : '',
                                  numberOfLinesDescription: 1,
                                  onSelected: () => interceptAnonymousUser(() => navigateToQuickAction()),
                                  shouldShowSubscriptRightAvatar: ReportUtils.isPolicyExpenseChat(quickActionReport),
                              },
                          ]
                        : []),
                ]}
                withoutOverlay
                anchorRef={fabRef}
            />
            <FloatingActionButton
                accessibilityLabel={translate('sidebarScreen.fabNewChatExplained')}
                role={CONST.ROLE.BUTTON}
                isActive={isCreateMenuActive}
                ref={fabRef}
                onPress={toggleCreateMenu}
            />
        </View>

What alternative solutions did you explore? (Optional)

N.A.

shawnborton commented 5 months ago

Ah, I wasn't thinking we'd change the names but that's a good point about @kevinksullivan's doc. Do we want to wait for that doc to update the icons here too?

shawnborton commented 5 months ago

If we didn't do any name changes but just changed out some icons, I think we'd get this as the correct image for the original comment here: CleanShot 2024-04-15 at 13 09 22@2x

shawnborton commented 5 months ago

I'll go ahead and update the issue for now.

ShridharGoel commented 5 months ago

Proposal

Updated

ghost commented 5 months ago

Updated Proposal as per comment

shubham1206agra commented 5 months ago

I'll go ahead and update the issue for now.

Thanks Now, the issue looks separated.

mountiny commented 5 months ago

Lets do this change after we update the copy. @eVoloshchak could you please triage the proposal for updating the icons only? the copy changes will be done in separate issue

trjExpensify commented 5 months ago

Yeah, I think doing icons right away now is fine. I was just hesitant on the menu item names as that has more implications throughout the flows in a number of places.

eVoloshchak commented 5 months ago

Use the new method to get the icon for the actions in the Composer "Plus" context menu here (t was also not mentioned in the OP, but it needs to be updated to be consistent)

This is an important detail, good catch, @tienifr. It isn't mentioned in the OP, but I think we definitely should update it for consistency. @shawnborton, could you please confirm the icons in the Composer "Plus" context menu also need to be updated?

dubielzyk-expensify commented 5 months ago

Great shout. I'll let @shawnborton (and @dannymcclain ) chime in, but I think it would make lots of sense to update them as well.

dannymcclain commented 5 months ago

Totally agree—we should definitely update them all to be consistent. Good catch.

mountiny commented 5 months ago

@eVoloshchak what are the next steps here?

eVoloshchak commented 5 months ago

In that case, I think we should proceed with @tienifr's proposal since it's the only proposal that covers all of the cases

🎀👀🎀 C+ reviewed!

melvin-bot[bot] commented 5 months ago

Current assignees @puneetlath and @mountiny are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 5 months ago

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

tienifr commented 5 months ago

@shawnborton

For Track expense, we want to use the coins icon For Scan in the money request flows, we want to use the scan receipt icon

I cannot find these two icons in app. Can you help check again as you said "Note that all of these icons already exist in the App repo"

shawnborton commented 5 months ago

For the coins, looks like we have it as "tax.svg" - I think renaming it to coins makes more sense personally. File is here.

Here's the icon you need for receipt scan: receipt-scan.svg.zip

shubham1206agra commented 5 months ago

@tienifr Please proceed with the PR now. We have merged the changes to unblock this issue.

dannymcclain commented 5 months ago

For the coins, looks like we have it as "tax.svg" - I think renaming it to coins makes more sense personally.

Let's please rename it to coins.svg if possible. And if we do, we should check and see where it's referenced to make sure we don't bork anything!

tienifr commented 5 months ago

@eVoloshchak PR https://github.com/Expensify/App/pull/40553 is ready

melvin-bot[bot] commented 4 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 4 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.68-3 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/40553

If no regressions arise, payment will be issued on 2024-05-08. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@eVoloshchak requires payment through NewDot Manual Requests
@tienifr requires payment automatic offer (Contributor)

melvin-bot[bot] commented 4 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@eVoloshchak] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@eVoloshchak] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@eVoloshchak] Determine if we should create a regression test for this bug.
[ ] [@eVoloshchak] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

puneetlath commented 4 months ago

@tienifr has been paid.

@eVoloshchak bump on the checklist for you.

eVoloshchak commented 4 months ago

@puneetlath, not sure if checklist is applicable here, this isn't a bug, we just needed to update the icons There is no PR that has caused this, no discussion needed and no regression test

puneetlath commented 4 months ago

Ah ok, that makes sense.

Payment summary:

$250 - C - paid via Upwork
$250 - C+ - @eVoloshchak - to be paid via NewDot

@eVoloshchak please request on NewDot. Thanks everyone!

JmillsExpensify commented 4 months ago

$250 approved for @eVoloshchak

Expensify / App

[HOLD for payment 2024-05-08] [$250] Update icons in global create and money request flows #40225

Feature Request

Platforms:

Screenshots/Videos

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Proposal