[WAITING ON CHECKLIST] Update the workspace connected bank account page for clarity and consistency

anmurali commented 2 months ago

Users are confused when they go to add a bank account to their workspace and cannot select their linked personal bank accounts. So let's add some explainer text that educates them on the need for a Business Bank Account. While we're at it, let's also update the page to our newest pattern for consistency.

Today

Update to

Final copy for the note:

Note: Personal bank accounts can't be used for payments on workspaces.

cc @shawnborton @maddylewis @jamesdeanexpensify

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a8923a1af5c75af2
Upwork Job ID: 1824502151817768637
Last Price Increase: 2024-08-23
Automatic offers:

BhuvaneshPatil | Contributor | 103689249

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01a8923a1af5c75af2

melvin-bot[bot] commented 2 months ago

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

cretadn22 commented 2 months ago

Proposal

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

Update the workspace connected bank account page for clarity and consistency (New design)

What is the root cause of that problem?

New design

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

We need to update this Section Component https://github.com/Expensify/App/blob/279f561c90adef037b27f73ef96c8487e44d0be8/src/pages/ReimbursementAccount/BankAccountStep.tsx#L128 to

<Section
       title={translate('workspace.bankAccount.streamlinePayments')}
       subtitle={translate('bankAccount.toGetStarted')}
       subtitleMuted
       illustration={LottieAnimations.Safe}.  // Need to use the new design 
       titleStyles={styles.titleStyle}  // Need to define
       childrenStyles={styles.childrenStyles}  // Need to define
>

Also need to add new note (detai in the PR)

What alternative solutions did you explore? (Optional)

Nodebrute commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-17T14:15:00Z.

Proposal

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

Add a note

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 can add following styles here. We should conditionally render these styles and display this note only when the user has added a personal bank account. We can use this key to check if plaidAccountID is stored in Onyx.

 key: ONYXKEYS.PERSONAL_BANK_ACCOUNT

     <View style={[styles.flexRow, styles.mb4, styles.alignItemsCenter, styles.pb1, styles.pt1]}>
                        <Icon
                            src={Expensicons.Lightbulb}
                            fill={theme.icon}
                            additionalStyles={styles.mr2}
                            small
                        />
                        <Text
                            style={[styles.textLabelSupportingNormal]}
                            suppressHighlighting
                        >
                            {translate('note')} // we can add note here
                        </Text>
                    </View>

We should also add the note in en.ts and es.ts file

Further styles can be modified according to the design in the PR.

We can update this section here. Additionally, I think we should add the illustrationBackgroundColor to align with the design.

 <Section
                        illustration={LottieAnimations.FastMoney} //Animation of our choice
                        title={translate('workspace.bankAccount.streamlinePayments')}
                        subtitle={translate('bankAccount.toGetStarted')}
                        subtitleMuted
                    >

[!NOTE]
As noted by a fellow contributor regarding the use of isCentralPane, I believe we should avoid using it. This prop is specifically for when the section is located in the central pane of the layout.

The main issue here is that we’re only checking isCentralPane, but we should also be checking for illustration. We need to update the check to (isCentralPane || !!illustration). https://github.com/Expensify/App/blob/76158981580aa046592df17c940fb6b289b0924d/src/components/Section/index.tsx#L173

Additionally, I noticed that the design uses the same styles for 'Connect Manually' and 'Connect with Plaid'. Therefore, we should also update the styles for 'Connect with Plaid' to match.

All the font sizes and other styles can be further adjusted in the pull request.

What alternative solutions did you explore? (Optional)

BhuvaneshPatil commented 2 months ago

Proposal

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

Update the workspace connected bank account page for clarity and consistency [Design Update]

What is the root cause of that problem?

New Feature

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

We should use Section component's prop to apply new design. https://github.com/Expensify/App/blob/c7d4c5ff253a0d05b1ca9046787064b5f8187a27/src/pages/ReimbursementAccount/BankAccountStep.tsx#L128-L131
```
<Section
title={translate("workspace.bankAccount.streamlinePayments")}
illustration={LottieAnimations.Coin}
isCentralPane
subtitle={translate("bankAccount.toGetStarted")}
subtitleMuted
></Section>;
```
By passing illustration instead of icon, we can achieve given design. With this prop, we need to add extra prop isCentralPane (which is missing in both proposals), and if it's passed the content doesn't have padding. With these props we need to pass subtitle prop as well, with subtitleMuted.

For showing new content (one with Bulb icon in design), we will need to add following view - Inside

component (styles can be modified according to design)


<View
style={[
styles.flexRow,
styles.mv3,
styles.alignItemsStart,
styles.gap2,
styles.mln1,
]}
>
<Icon
src={Expensicons.Lightbulb}
height={variables.iconSizeLarge}
width={variables.iconSizeLarge}
fill={theme.icon}
additionalStyles={styles.p1}
/>
<Text style={[styles.textLabelSupporting, styles.textAlignLeft]}>
Lorem ipsum dolor sit amet consectetur adipisicing elit. Eligendi fugiat
facilis nisi alias
</Text>
</View>;



Result - 
<img width="388" alt="Screenshot 2024-08-17 at 2 57 19 AM" src="https://github.com/user-attachments/assets/beecce5b-64b2-44d9-995f-e77b3a114612">

<details>
<summary>
Without isCentralPane
</summary>
<img width="458" alt="Screenshot 2024-08-17 at 2 58 26 AM" src="https://github.com/user-attachments/assets/ebc8289f-c08a-4162-9641-5f8b4a6d230c">

</details>

### What alternative solutions did you explore? (Optional)

shawnborton commented 2 months ago

Just adding a note that we do have an existing lottie animation to use in the top green area of the card, so it will match other places in our app (like in Settings) where the header area is animated.

Ollyws commented 2 months ago

Looking at the timestamps of the proposal edits it seems that @BhuvaneshPatil had the first fully fleshed-out proposal so lets go with them. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Nodebrute commented 2 months ago

@Ollyws When @BhuvaneshPatil proposed the solution, the only difference between his proposal and mine was the usage of isCentralPane, which was incorrect in his proposal. The main difference between our proposals was indeed the usage of isCentralPane. I later pointed out the correct root cause analysis (RCA). Could you please review the proposal again?

Nodebrute commented 2 months ago

Hey @Ollyws, I wanted to clarify that isCentralPane isn't applicable here, as it is specifically meant for central panel layouts. I was the first to propose the note code and subsequently suggested the correct approach for adding animation.

BhuvaneshPatil commented 2 months ago

@Nodebrute By looking at timestamps, when I posted my proposal, your proposal didn't mention about updating /adding illustration prop in. <Section/> component

Nodebrute commented 2 months ago

@BhuvaneshPatil Hey, I missed that we were also supposed to add the illustration. I've since updated my code to include the illustrations, but my approach is quite different from yours. Just like in your proposal, it looks like you missed mentioning the 'Connect Online with Plaid' button

Nodebrute commented 2 months ago

Hey @Ollyws, here's a timeline of all the proposals:

@cretadn22 was the first to mention the section code.
I then introduced the note code.
@BhuvaneshPatil's proposal addressed both the section and note codes and pointed out that the isCentralPane was missing from the previous proposals.
I subsequently updated my proposal by adding a note explaining why we should avoid using isCentralPane and correct section code. I also added the change for the 'Connect Online with Plaid' button.

Nodebrute commented 2 months ago

Hi @Ollyws, just a gentle reminder to let me know your thoughts on this when you have a moment. Thanks!

tgolen commented 2 months ago

Hey, I'm just seeing this issue, but whoever works on this, can you please only make the new "Note" visible when the user has a personal bank account connected? I don't think the note needs to be visible if they haven't ever added a personal bank account and it could be just as confusing.

Nodebrute commented 2 months ago

Proposal updated with the new requested changes.

melvin-bot[bot] commented 2 months ago

@francoisl, @Ollyws Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

francoisl commented 2 months ago

Hi @Ollyws, can confirm the chosen proposal is still valid please, especially given the last request to only make the note visible after an account is connected, and also the discussion about using isCentralPane above in this comment?

Ollyws commented 2 months ago

I still stand by my initial selection of @BhuvaneshPatil

It was the first and only proposal that suggested adding all the required features including the illustration prop. Although it was the second proposal to outline usage of the Section component, the first proposal to mention it would have caused a regression due to lack of the isCentralPane prop.

@Nodebrute then built upon @BhuvaneshPatil’s proposal suggesting we further modify what is essentially the functionality of the isCentralPane prop to check for the illustration, while it may be a nice addition it does not constitute a new, unique proposal in itself.

As for this note, it was added after I had selected the proposal so it can be considered a note addressed to the selected contributor for consideration in their PR.

melvin-bot[bot] commented 2 months ago

📣 @BhuvaneshPatil 🎉 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 📖

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @kadiealexander (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.

trjExpensify commented 2 months ago

Adding the Bug label to get a BZ assigned. Also, @BhuvaneshPatil can you provide an ETA for the PR? Thanks!

melvin-bot[bot] commented 2 months ago

@francoisl, @Ollyws, @BhuvaneshPatil Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 months ago

@francoisl @Ollyws @BhuvaneshPatil @kadiealexander this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

BhuvaneshPatil commented 2 months ago

@trjExpensify I am facing trouble testing on IOS device. I will raise PR as soon as it works

trjExpensify commented 2 months ago

Have you taken your questions to #expensify-open-source to get some help on what's blocking you from testing on iOS?

maddylewis commented 2 months ago

@BhuvaneshPatil - bump on Tom's question when you have a sec! https://github.com/Expensify/App/issues/47578#issuecomment-2325341777

BhuvaneshPatil commented 2 months ago

PR is in review

BhuvaneshPatil commented 2 months ago

@anmurali

While working on PR I found out that this page is also using old design shall we cover this page as well.

I would love to work on that as well. Let me know what you think.

shawnborton commented 1 month ago

I think following up to fix that particular spot makes sense for consistency's sake.

I also think something we missed when implementing this is that we should use our standard edge-to-edge push rows here. Right now we use the old "rounded pill" option row style which I think we should kill:

https://github.com/user-attachments/assets/5aea28cd-1693-4619-8e32-018afab7f14e

The correct style looks like this:

https://github.com/user-attachments/assets/5204a320-7faa-4b92-bae3-e3ad9eb9ed26

trjExpensify commented 1 month ago

@BhuvaneshPatil can you work on a PR to address Shawn's feedback please, and then tag Design for review? Thanks!

BhuvaneshPatil commented 1 month ago

Sure. I will raise the PR.

@trjExpensify what do you think about this comment

BhuvaneshPatil commented 1 month ago

raised the PR, uploaded video for macos web. will update videos on other platforms next morning

trjExpensify commented 1 month ago

I agree with Shawn:

I think following up to fix that particular spot makes sense for consistency's sake.

BhuvaneshPatil commented 1 month ago

@trjExpensify @shawnborton PR is now merged 🎊

trjExpensify commented 1 month ago

Great!

BhuvaneshPatil commented 1 month ago

The PR was deployed to production on Sep 27 https://github.com/Expensify/App/pull/49490#issuecomment-2379773696

BhuvaneshPatil commented 1 month ago

@kadiealexander Can you please take a look

kadiealexander commented 4 weeks ago

Sorry for missing this one guys!

kadiealexander commented 4 weeks 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:

[ ] [@Ollyws] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@Ollyws] 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:
[ ] [@Ollyws] 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:
[ ] [@Ollyws] Determine if we should create a regression test for this bug.
[ ] [@Ollyws] 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.
[ ] [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

kadiealexander commented 4 weeks ago

Payouts due:

[x] Contributor: $250 @BhuvaneshPatil (Offer link)
[ ] Reviewer: $500 @Ollyws (via manual requests)

Please see this comment for additional context

Upwork job is here.

melvin-bot[bot] commented 2 weeks ago

@francoisl, @Ollyws, @BhuvaneshPatil, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 weeks ago

@francoisl, @Ollyws, @BhuvaneshPatil, @kadiealexander Eep! 4 days overdue now. Issues have feelings too...

kadiealexander commented 2 weeks ago

@Ollyws bump on the checklist! Reassigning someone to action the BugZero step on the checklist as I'm heading OOO.

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @jliexpensify (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.

Ollyws commented 2 weeks ago

Is the second PR being treated as within the scope of the original issue or is that due payment aswell?

jliexpensify commented 1 week ago

I might get @francoisl to weigh in when he's back. I'm sorry, but I just got assigned this issue, so I don't have any context.

I'm happy to amend the Payment Summary, but it also might be easier to have a second job for payment? (for auditing/record keeping sake)

francoisl commented 1 week ago

It looks to me like the changes from the second PR weren't included in the scope of the original issue, so that would mean it's also due for payment.

Ollyws commented 6 days ago

I don't know why this was closed, but @jliexpensify could you have a look at the above comment, thanks.

Expensify / App