Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-07-23] [$250] [Search v1] Design updates to Search rows on mobile #44563

Closed
shawnborton opened this issue Jun 27, 2024 · 17 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@shawnborton
Copy link
Contributor

shawnborton commented Jun 27, 2024

We have a few simplifications we'd like to make for the mobile view of Search rows to help make the rows a bit more digestible and legible:
CleanShot 2024-06-27 at 20 14 47@2x

  1. Let's remove the tag from the bottom line under the merchant, and only show a Category if one is available
  2. Let's remove the icon in front of Category
  3. If no category exists, let's vertically center the Merchant line in the available space
  4. If no Merchant exists, let's use the Description if it exists. If neither exists, do nothing and leave it blank

cc @JmillsExpensify @luacmartins @Expensify/design

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b532850f6d400dbe
  • Upwork Job ID: 1806412035454885668
  • Last Price Increase: 2024-06-27
  • Automatic offers:
    • dukenv0307 | Reviewer | 102915283
    • dominictb | Contributor | 102915284
Issue OwnerCurrent Issue Owner: @garrettmknight
@luacmartins luacmartins changed the title Design updates to Search rows on mobile [Search v1] Design updates to Search rows on mobile Jun 27, 2024
@luacmartins luacmartins added Daily KSv2 NewFeature Something to build that is a new item. External Added to denote the issue can be worked on by a contributor labels Jun 27, 2024
Copy link

melvin-bot bot commented Jun 27, 2024

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

@melvin-bot melvin-bot bot changed the title [Search v1] Design updates to Search rows on mobile [$250] [Search v1] Design updates to Search rows on mobile Jun 27, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 27, 2024
Copy link

melvin-bot bot commented Jun 27, 2024

Triggered auto assignment to @garrettmknight (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jun 27, 2024
Copy link

melvin-bot bot commented Jun 27, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 27, 2024
@neonbhai
Copy link
Contributor

neonbhai commented Jun 27, 2024

Proposal

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

Design updates to Search rows on mobile

What is the root cause of that problem?

This is a feature request

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

Let's remove the tag from the bottom line under the merchant, and only show a Category if one is available

We will remove the Tag Cell from here:

<TagCell
showTooltip={showTooltip}
transactionItem={item}
isLargeScreenWidth={isLargeScreenWidth}
/>

Let's remove the icon in front of Category

We will edit this function to only use TextWithTooltip. We will remove this part. We will update the styles to use only one component if it can be done simply.

If no category exists, let's vertically center the Merchant line in the available space

We will add alignItemsCenter styling to this view if transactionItem?.category doesn't exist. We will use a variable to facilitate the check. The line seems near centre but not on, so we apply this.

If no Merchant exists, let's use the Description if it exists. If neither exists, do nothing and leave it blank

We will add a check here to see if transactionItem.formattedMerchant exists. We will render the description if it doesn't.

let merchant = transactionItem.shouldShowMerchant ? transactionItem.formattedMerchant : description;

Assuming we want this behaviour on web also. If not we will add a check for isLargeScreenWidth when defaulting to description

@dominictb
Copy link
Contributor

dominictb commented Jun 28, 2024

Proposal

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

  • [Search v1] Design updates to Search rows on mobile

What is the root cause of that problem?

  • New feature.

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

  1. With the feature:

Let's remove the tag from the bottom line under the merchant, and only show a Category if one is available

  • We can update:
    <View style={[styles.flex2, styles.gap1]}>
    <MerchantCell
    transactionItem={item}
    showTooltip={showTooltip}
    isLargeScreenWidth={false}
    />
    <View style={[styles.flexRow, styles.flex1, styles.alignItemsEnd, styles.gap3]}>
    <CategoryCell
    isLargeScreenWidth={isLargeScreenWidth}
    showTooltip={showTooltip}
    transactionItem={item}
    />
    <TagCell
    showTooltip={showTooltip}
    transactionItem={item}
    isLargeScreenWidth={isLargeScreenWidth}
    />
    </View>

    to:
+ <View style={[styles.flex2, !item.category && styles.justifyContentCenter, styles.gap1]}>
                        <MerchantCell
                            transactionItem={item}
                            showTooltip={showTooltip}
                            isLargeScreenWidth={false}
                        />
+                        {item.category && (
+                            <View style={[styles.flexRow, styles.flex1, styles.alignItemsEnd]}>
+                                <CategoryCell
+                                    isLargeScreenWidth={isLargeScreenWidth}
+                                    showTooltip={showTooltip}
+                                    transactionItem={item}
+                                />
+                            </View>
+                        )}
                    </View>
  1. With the feature:

Let's remove the icon in front of Category

+       <TextWithTooltip
            shouldShowTooltip={showTooltip}
            text={transactionItem?.category}
            style={[styles.textMicro, styles.mnh0]}
        />
  • In above, we only remove the icon, the text style should not be changed.
  1. With the feature:

If no category exists, let's vertically center the Merchant line in the available space

  • We already do it in 1.
  1. With the feature:

If no Merchant exists, let's use the Description if it exists. If neither exists, do nothing and leave it blank

    let merchant = transactionItem.shouldShowMerchant ? transactionItem.formattedMerchant : description;
+  if (!isLargeScreenWidth) {
+        merchant = transactionItem.shouldShowMerchant ? (transactionItem.formattedMerchant ?
+   transactionItem.formattedMerchant : description) : description;
+    }

then update:


to:

                        isLargeScreenWidth={true}
  • In the above, we only display the description as the fallback of the merchant in small screen width and shouldn't do it in large screen width because, in that screen, we have a column named "merchant". In this case, displaying the description instead of the merchant will confuse the user. But it is just my point, we can still apply the same feature on both screens based on the design team's decision.

What alternative solutions did you explore? (Optional)

@dominictb
Copy link
Contributor

Proposal Updated

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jun 28, 2024

Thanks for all your proposals.

@neonbhai Your proposal is first but using alignItemsCenter is not correct to me. It should be justifyContentCenter like @dominictb's proposal.

I also agree we should set isLargeScreenWidth={true} in

At this place, isLargeScreenWidth is already true so setting isLargeScreenWidth: false can cause confusion.

Let's go with @dominictb's proposal

BTW, should we apply the description fallback on web/desktop? If yes, do we need to change the merchant column name? @shawnborton @luacmartins

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jun 28, 2024

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 28, 2024
Copy link

melvin-bot bot commented Jun 28, 2024

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

Offer link
Upwork job

Copy link

melvin-bot bot commented Jun 28, 2024

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

@luacmartins
Copy link
Contributor

BTW, should we apply the description fallback on web/desktop? If yes, do we need to change the merchant column name? @shawnborton @luacmartins

I'll let @shawnborton confirm this one since he's more on top of those discussions.

@shawnborton
Copy link
Contributor Author

shawnborton commented Jun 28, 2024

BTW, should we apply the description fallback on web/desktop? If yes, do we need to change the merchant column name?

I think we decided no, since we plan to implement the dynamic columns on web/desktop - right @luacmartins ?

@luacmartins
Copy link
Contributor

Yea, that makes sense!

@dominictb
Copy link
Contributor

@dukenv0307 PR #44677 is ready

@melvin-bot melvin-bot bot added Overdue Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 Overdue labels Jul 1, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 16, 2024
@garrettmknight
Copy link
Contributor

@luacmartins is this one ready to be paid out? I'm not 100% sure if there's anything else we're waiting on or if the automation just didn't run.

@luacmartins
Copy link
Contributor

Yes, this is ready for payment

@luacmartins luacmartins changed the title [$250] [Search v1] Design updates to Search rows on mobile [HOLD for payment 2024-07-23] [$250] [Search v1] Design updates to Search rows on mobile Jul 23, 2024
@luacmartins luacmartins added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Jul 23, 2024
@garrettmknight garrettmknight added Daily KSv2 and removed Weekly KSv2 labels Jul 23, 2024
@garrettmknight
Copy link
Contributor

Payment Summary:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Archived in project
Development

No branches or pull requests

6 participants