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

[$250] Web - Expensify Card - Expensify Card page in empty state has two scoll bars #56690

Open
1 of 8 tasks
vincdargento opened this issue Feb 11, 2025 · 48 comments
Open
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@vincdargento
Copy link

vincdargento commented Feb 11, 2025

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.96-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Windows 11 / Chrome
App Component: Workspace Settings

Action Performed:

Precondition:

  • Expensify Card feature is set up but no card is assigned.
  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Expensify Card.

Expected Result:

There will be one scroll bar in Expensify Card page in empty state.

Actual Result:

There are two scroll bars in Expensify Card page in empty state.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021889430675865762724
  • Upwork Job ID: 1889430675865762724
  • Last Price Increase: 2025-02-18
  • Automatic offers:
    • twilight2294 | Contributor | 106202611
Issue OwnerCurrent Issue Owner: @hoangzinh
@vincdargento vincdargento added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

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.

@FitseTLT
Copy link
Contributor

Proposal

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

Web - Expensify Card - Expensify Card page in empty state has two scoll bars

What is the root cause of that problem?

We are adding unnecessary scroll view in here

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

We should remove it as there is a scroll view in the parent component

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N / A

What alternative solutions did you explore? (Optional)

Copy link
Contributor

⚠️ @nkdengineer Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@twilight2294
Copy link
Contributor

twilight2294 commented Feb 11, 2025

Proposal

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

Expensify Card page in empty state has two scoll bars

What is the root cause of that problem?

We do not pass showsVerticalScrollIndicator={false} to EmptyStateComponent in EmptyCardView:

<EmptyStateComponent
SkeletonComponent={CardRowSkeleton}
headerMediaType={CONST.EMPTY_STATE_MEDIA.ILLUSTRATION}

This causes us to show double scrollbar

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

We should pass showsVerticalScrollIndicator={false} to EmptyStateComponent, and keep the scrollview in EmptyCardView as it is.

showsVerticalScrollIndicator tells the component whether or not to show the verticalScrollIndicator:

Image

EmptyCardView also has a text component:

<Text style={[styles.textMicroSupporting, styles.m5]}>{translate('workspace.expensifyCard.disclaimer')}</Text>
</ScrollView>

So we need ScrollView for this page.

I think we can totally disable the scroll inside EmptyStateComponent as we already scroll the page in EmptyCardView, this can be done with the help of scrollEnabled prop, we need to introduce a new optional prop for this though, then pass the value as false to EmptyStateComponent which will pass this same value to ScrollView component in EmptyStateComponent:

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A This is a UI bug

What alternative solutions did you explore? (Optional)

  1. Remove scrollView from the following places:

Replace the use of scrollview with a View

  1. Wrap EmptyCardView component with a scrollview:
                <ScrollView>
                    <EmptyCardView isBankAccountVerified={isBankAccountVerified} />
                </ScrollView>

{isEmptyObject(cardsList) ? (
<EmptyCardView isBankAccountVerified={isBankAccountVerified} />
) : (

Result:

Screen.Recording.2025-02-13.at.8.26.57.AM.mov

@jliexpensify
Copy link
Contributor

Woah, weird! @Expensify/design - just confirming: is this an Internal issue?

@dannymcclain
Copy link
Contributor

Oh boy—I honestly don't know. @shawnborton you have any ideas? It seems to be somehow related to the empty state + disclaimer message combo.

@shawnborton
Copy link
Contributor

I would think this can be fixed Externally. No idea why it is happening, but we should definitely fix it!

@shawnborton
Copy link
Contributor

cc @fabioh8010 as I think you worked on some of the empty state stuff? I could be making that up though!

@jliexpensify
Copy link
Contributor

Thanks Danny + Shawn, gonna drop the External label then.

@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

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

@melvin-bot melvin-bot bot changed the title Web - Expensify Card - Expensify Card page in empty state has two scoll bars [$250] Web - Expensify Card - Expensify Card page in empty state has two scoll bars Feb 11, 2025
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

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

@mosesbabu
Copy link

mosesbabu commented Feb 12, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-12 19:03:39 UTC.

Proposal

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

The Expensify Card page in empty state currently displays two scroll bars, which creates a poor user experience and violates UI/UX best practices. This issue appears on the workspace settings > Expensify Card page when no card is assigned.

What is the root cause of that problem?

The root cause is the unnecessary addition of a <ScrollView> component in the empty state view at:
This creates redundant scrolling functionality since there is already a scroll view implemented in the parent component.

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

We should remove the <ScrollView> wrapper from the EmptyCardView component, as the parent component already provides the necessary scrolling functionality. The content should be rendered directly within its container without the additional scroll wrapper.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

No alternative solutions were explored as the issue has a clear cause and straightforward fix - removing redundant scrolling functionality.

@hoangzinh
Copy link
Contributor

@FitseTLT Thanks for posting proposal.

We should remove it as there is a scroll view in the parent component

Can you clarify it? Or link me to the LOC of scroll view in the parent component. Thank you. Btw, your solution doesn't work properly, it doesn't show "disclaimer" text at the bottom although I already scroll to bottom

Click here to view screenshot of your solution

Image

@hoangzinh
Copy link
Contributor

@twilight2294 Thanks for posting proposal. I tried your solution but it seems sometimes it's hard to scroll to the bottom of the page.

Click here to view recording of your solution
Screen.Recording.2025-02-12.at.20.19.54.mov

@hoangzinh
Copy link
Contributor

@mosesbabu Thanks for posting proposal. Please ensure your proposal follows the template here https://github.com/Expensify/App/blob/main/contributingGuides/PROPOSAL_TEMPLATE.md

@cretadn22
Copy link
Contributor

cretadn22 commented Feb 12, 2025

Proposal

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

Expensify Card page in empty state has two scoll bars

What is the root cause of that problem?

We are utilizing two ScrollViews in this section

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

We should use only one ScrollView to improve the user experience.

remove the ScrollView and View here

<ScrollView>
<View style={[{height: windowHeight - headerHeight}, styles.pt5]}>

remove this footer

<Text style={[styles.textMicroSupporting, styles.m5]}>{translate('workspace.expensifyCard.disclaimer')}</Text>

we will pass two new props to the EmptyStateComponent

footer={(<Text style={[styles.textMicroSupporting, styles.m5]}>{translate('workspace.expensifyCard.disclaimer')}</Text>
)}
innerContentStyle={{{height: windowHeight - headerHeight}, styles.pt5}}

update the EmptyStateComponent accordingly.

<ScrollView>
            <View style={innerContentStyle}>
                 // This is all current element ...
            </View>
            {footer}
</ScrollView>
Screen.Recording.2025-02-12.at.20.52.48.mov

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

N/A
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@fabioh8010
Copy link
Contributor

cc @fabioh8010 as I think you worked on some of the empty state stuff? I could be making that up though!

Hey @shawnborton I don't remember working on this xD but I'm checking internally if someone had in the past.

@twilight2294
Copy link
Contributor

@twilight2294 Thanks for posting proposal. I tried your solution but it seems sometimes it's hard to scroll to the bottom of the page.

Click here to view recording of your solution
Screen.Recording.2025-02-12.at.20.19.54.mov

@hoangzinh thanks for the feedback, I have updated my proposal, can you please review it again? thanks

@hoangzinh
Copy link
Contributor

@cretadn22 @twilight2294 thanks for update & posting proposals. I'm still considering proposals. Btw, do we know why it happened? It used to work in the beginning #45690 (comment)

@cretadn22
Copy link
Contributor

@hoangzinh Based on the initial idea, we don't want to display "The Expensify Visa® Commercial Card is..." upfront, requiring users to scroll down to see the disclaimer. That's why we use a scroll view in EmptyCardView.

@cretadn22
Copy link
Contributor

With my proposal, we only need to use a scroll view but it will still function properly

@Krishna2323
Copy link
Contributor

Proposal

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

Web - Expensify Card - Expensify Card page in empty state has two scroll bars

What is the root cause of that problem?

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

  • We just need to disable the ScrollView of the EmptyStateComponent -- the children component. For this we can pass a prop scrollEnabled={false} from EmptyCardView to EmptyStateComponent and then scrollEnabled={scrollEnabled} to the ScrollView of EmptyStateComponent.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A -- UI issue

What alternative solutions did you explore? (Optional)

  • We can render the disclaimer message in the empty view list card using the children prop of EmptyStateComponent.

@twilight2294
Copy link
Contributor

do we know why it happened? It used to work in the beginning #45690 (comment)

sorry i don't have the complete idea here, but my proposed solution makes a single use of scrollview which is better to maintain in long term IMO, what do you think ?

@cretadn22
Copy link
Contributor

@hoangzinh EmptyStateComponent is a shared component used in multiple places. Removing the ScrollView from it would require adding ScrollView manually to all instances where this component is used.

@cretadn22
Copy link
Contributor

With my solution, EmptyStateComponent becomes more versatile, as it now supports displaying footer content as well.

@hoangzinh
Copy link
Contributor

Yep, I did consider your suggestion @cretadn22. But I think that what if we need to put another component/view in the top of EmptyStateComponent, then we have to put another prop like "header". Therefore I think if we remove ScrollView out of EmptyStateComponent and then leave it to the parent component, then it will solve most of the cases. Agree that it's a pain atm that we need to update current using places of EmptyStateComponent but it's better for the long-term.

Copy link

melvin-bot bot commented Feb 18, 2025

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

@twilight2294
Copy link
Contributor

friendly bump for assignment @robertjchen

@twilight2294
Copy link
Contributor

friendly bump for assignment @robertjchen , summary here

@jliexpensify
Copy link
Contributor

Have DM-ed Robert as well, thanks @twilight2294!

@robertjchen
Copy link
Contributor

Reviewed, let's go with @twilight2294 's approach 👍

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 20, 2025
Copy link

melvin-bot bot commented Feb 20, 2025

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

@twilight2294
Copy link
Contributor

Will raise the PR tomorrow

@melvin-bot melvin-bot bot added the Overdue label Feb 23, 2025
@mosesbabu
Copy link

mosesbabu commented Feb 23, 2025 via email

@jliexpensify
Copy link
Contributor

This has already been assigned @mosesbabu - you'll need to search for jobs with the Help Wanted label if you wish to apply.

@robertjchen robertjchen added the Reviewing Has a PR in review label Feb 24, 2025
@melvin-bot melvin-bot bot removed the Overdue label Feb 24, 2025
@twilight2294
Copy link
Contributor

twilight2294 commented Feb 26, 2025

PR will be up tomorrow morning, Draft here

@twilight2294
Copy link
Contributor

Draft PR is ^, Facing a little delay because we would need to update all places where we use EmptyStateComponent, but it should be ready for review over the weekend

Copy link

melvin-bot bot commented Mar 5, 2025

@hoangzinh Eep! 4 days overdue now. Issues have feelings too...

@hoangzinh
Copy link
Contributor

how is it going? @twilight2294

@twilight2294
Copy link
Contributor

sorry for not updating here, ready almost, i am testing for the components related to the empty state , should i open it up for review now?

@hoangzinh
Copy link
Contributor

No, you can open the PR when everything is ready for review.

@twilight2294
Copy link
Contributor

Done with testing, I will upload videos and open up the PR tomorrow morning IST

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Mar 9, 2025
@twilight2294
Copy link
Contributor

PR is ready for your review @hoangzinh , apologies for this delay:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: LOW
Development

No branches or pull requests