Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

[$250] Tab bar in create expense flows uses incorrect icon size #49577

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month 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!


Version Number: 9.0.39-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @shawnborton Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1726810593632939

Action Performed:

  1. Go to staging.new.expensify.com
  2. Create a submit expense flow
  3. Observe the Tab bar

    Expected Result:

    The tab bar size is standard on all tabs

    Actual Result:

    In the tab bar for the create expense flows, the icon is at 20x20. However, the tab bar across the top of the Search pages uses an icon that is 16x16

    Workaround:

    Unknown

    Platforms:

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

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

Screenshots/Videos

CleanShot 2024-09-20 at 07 35 43@2x

CleanShot 2024-09-20 at 07 36 39@2x

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838269032808976067
  • Upwork Job ID: 1838269032808976067
  • Last Price Increase: 2024-09-23
  • Automatic offers:
    • ShridharGoel | Contributor | 104165006
Issue OwnerCurrent Issue Owner: @thesahindia
melvin-bot[bot] commented 1 month ago

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

MuaazArshad commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-22 15:34:26 UTC.

Proposal

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

Tab bar in create expense flows uses incorrect icon size

What is the root cause of that problem?

We are not passing large in button component. https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Search/SearchStatusBar.tsx#L181-L201

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

We can pass large here https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Search/SearchStatusBar.tsx#L181-L201

  1. Or we can add
    iconStyles={[{height: '20px'}, {width: '20px'}]}

    here https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Search/SearchStatusBar.tsx#L181-L201

    What alternative solutions did you explore? (Optional)

We can also set the size of icons decreasing the size in the submit expense tab by passing medium and hasText in TabIcon

https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/TabSelector/TabIcon.tsx#L18-L40 we will pass it in both TabIcon components for active and inactive tab.

abzokhattab commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-22 15:42:32 UTC.

Proposal

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

Tab bar in create expense flows uses incorrect text and icon sizes

What is the root cause of that problem?

we are not using the same text styling in both the submit expense and search selectors

https://github.com/Expensify/App/blob/d61c06ff45a9d7e4e1bec9aab0bca933e4e1d549/src/components/Search/SearchStatusBar.tsx#L181-L201

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

  1. we should update the icon size to be the normal size as in the TabIcon ... since the default icon size inside the button is enforced as medium, we can pass meduim={false}, this will make both sizes normal and consistent with the tab icon
  2. optionally we should add the styles.tabText prop to the textStyles to be the same as the one used for the tab bar of the submit expense
                        textStyles={(!isActive && StyleUtils.getTextColorStyle(theme.textSupporting), styles.tabText(isActive))}
  3. optionally we can also add the styles.tabSelector to the ScrollView styles inside the SearchStatusBar
POC image

What alternative solutions did you explore? (Optional)

If we want to standardize the icon size to 16x16 across both the search and expense flows, we need to update the expense flow to match the others. To achieve this, we need to add the following props to the TabIcon component:

hasText
medium

Since all tab icons include text, we need to enable the hasText flag, similar to how it's implemented in the Button component: https://github.com/Expensify/App/blob/c119850bda37ed2847cc5da6956650fcd9d66cbc/src/components/Button/index.tsx#L283-L285

Alternatively, we can add the hasText flag to the TabIcon props and set it to true when a title is present (hasText={!!text}) here .

This approach ensures the behavior remains consistent with the button component.

By doing this, the icon will scale up when no text is present in the future.

Result:

image
ShridharGoel commented 1 month ago

Proposal

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

Tab bar in create expense flows uses incorrect icon size.

What is the root cause of that problem?

Icon size is not being passed in TabIcon, so it takes the default medium size 20x20.

https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/TabSelector/TabIcon.tsx#L25-L28

https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/TabSelector/TabIcon.tsx#L31-L35

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

Pass small in those places:

<Animated.View style={{opacity: inactiveOpacity}}>
    <Icon
        src={icon}
        fill={theme.icon}
        small
    />
</Animated.View>
<Animated.View style={[StyleSheet.absoluteFill, {opacity: activeOpacity}]}>
    <Icon
        src={icon}
        fill={theme.iconMenu}
        small
    />
</Animated.View>
abzokhattab commented 1 month ago

Proposal updated

changed the icons size inside the header of the search page

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

thesahindia commented 1 month ago

@ShridharGoel's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

thesahindia commented 1 month ago

@garrettmknight, could you add the design label here? We need someone from the design team here.

I want to confirm if we want 16x16 icon size at "Start chat" page

Screenshot 2024-09-25 at 10 36 16 PM
MuaazArshad commented 1 month ago

We should increase the size of the search page since we're consistently using 20x20 across other areas as the standard. Additionally, reducing the icon size doesn't look good aesthetically.

Screenshot 2024-09-26 at 5 43 05 PM

Additionally, reducing the icon size doesn't look good aesthetically.

Gajendra-Gupta commented 1 month ago

Proposal

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

Tab bar in create expense flows uses incorrect icon size.

What is the root cause of that problem?

We are not adding iconStyle in button component. https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Search/SearchStatusBar.tsx#L181-L201

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

We need to add iconStyle={[{height: "20px", width: "20px"}]} in button component. https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Search/SearchStatusBar.tsx#L181-L201

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

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

abzokhattab commented 1 month ago

Should we get an approval from the design team on the expected sizes?

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.

jasperhuangg commented 1 month ago

@dubielzyk-expensify We need some help on this comment, thanks!

dannymcclain commented 1 month ago

I want to confirm if we want 16x16 icon size at "Start chat" page

Yes, ideally every tab in the app should use these styles.

Quick summary of styles:

Tab component in Figma

MuaazArshad commented 1 month ago

@thesahindia The alternative in my proposal is correct. It involves resizing the icon to 16px by passing medium in Icon because when the tab contains text, medium adjusts the icon to 16px. In contrast, passing small adjusts it to 12px. The selected proposal is incorrect as it sets the icon to 12px. Can you review it again, please?

thesahindia commented 1 month ago

@thesahindia The alternative in my proposal is correct. It involves resizing the icon to 16px by passing medium in Icon because when the tab contains text, medium adjusts the icon to 16px. In contrast, passing small adjusts it to 12px. The selected proposal is incorrect as it sets the icon to 12px. Can you review it again, please?

Could you share the screenshot after applying the changes?

abzokhattab commented 1 month ago

Proposal Updated

Updated my altenative proposal to have the hasText prop and to use same behavior as in the button component

dubielzyk-expensify commented 1 month ago

Yep, exactly what @dannymcclain said 😄

MuaazArshad commented 1 month ago

@thesahindia The alternative in my proposal is correct. It involves resizing the icon to 16px by passing medium in Icon because when the tab contains text, medium adjusts the icon to 16px. In contrast, passing small adjusts it to 12px. The selected proposal is incorrect as it sets the icon to 12px. Can you review it again, please?

Could you share the screenshot after applying the changes?

Screenshot 2024-10-01 at 6 39 05 PM
thesahindia commented 1 month ago

Could you also show the code along with the result? Like below?

Screenshot 2024-10-03 at 5 14 58 PM

It's not possible to get 16x16 by passing medium because we aren't passing hasText is false. How did you get this result are you just changing it in the browser?

thesahindia commented 1 month ago

@ShridharGoel, you are already assigned. Let me know once the PR is ready.

abzokhattab commented 1 month ago

is it possible please to reassess the proposals after Danny's clarification in this comment ?

The OP is unclear about whether we should increase the search icon size to match the submit icon, or if it should be the other way around.

Expected Result: The tab bar icon sizes should be consistent across all tabs.

Actual Result: In the tab bar for the create expense flows, the icon is at 20x20. However, the tab bar across the top of the Search pages uses an icon that is 16x16

so i think we should reassess the proposals after this comment.

thesahindia commented 1 month ago

Sorry but it's not possible.

I agree that the description isn't clear, but the title is self-explanatory. "Tab bar in create expense flows uses incorrect icon size"

melvin-bot[bot] commented 1 month ago

@garrettmknight @jasperhuangg @ShridharGoel @thesahindia @dubielzyk-expensify 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!

melvin-bot[bot] commented 1 week ago

This issue has not been updated in over 15 days. @garrettmknight, @jasperhuangg, @ShridharGoel, @thesahindia, @dubielzyk-expensify eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

garrettmknight commented 1 week ago

Still working through the PR, updating back to weekly.