Repository navigation
[HOLD for payment 2023-09-08] [$1000] Split money- participants detail hidden after clicking on show more button #24476
Description
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
1 Split money with multiple users
2 On the second page of split money, click on show more.
3 Verify that participants are hidden which can be scrolled.
Split money
4 Click on the split money box > this will open the split money details.
5 Click on show more
Expected Result:
I was expecting that the entire detail page would be scrollable when I clicked on the show more button. Reason being, participants' detail & amount are the most important components. But things like date and merchant are fixed rather than participant details.
Actual Result:
Gives the illusion that participants are hidden or deleted. No scrollbar. Sometimes, it's hard to scroll.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
Version Number: 1.3.53-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
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
Notes/Photos/Videos: Any additional supporting documentation
SPLIT.01.mp4
Recording.342.mp4
Expensify/Expensify Issue URL:
Issue reported by: @avi-shek-jha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691097988530299
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01eb2b30669469492f
- Upwork Job ID: 1692405727765413888
- Last Price Increase: 2023-08-25
- Automatic offers:
- DanutGavrus | Contributor | 26385876
- avi-shek-jha | Reporter | 26385878
Activity
melvin-bot commentedon Aug 12, 2023
Triggered auto assignment to @jliexpensify (
Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.melvin-bot commentedon Aug 12, 2023
Bug0 Triage Checklist (Main S/O)
Platformsin OP are ✅)jeet-dhandha commentedon Aug 12, 2023
It's intentional 👍, not a mistake. We added inline scrolling to address a problem: when more than 8 people are chosen, the main description gets hidden, and users have to scroll down to see it.
jeet-dhandha commentedon Aug 12, 2023
@jliexpensify - This might be a feature request but one way to solve this is to show 3 person and for all person we can add a "View All" / "View All Participants" / "All Participants" - button, which will open up a participant list including all the selected members. What are your thoughts on this one ?
jliexpensify commentedon Aug 14, 2023
Hmm yeah, I agree - it's a little annoying, but I don't see this as a huge deal.
That's an interesting idea - @jeet-dhandha could you propose that in the Open Source channel? For now, I'll close off this GH and if we decide to move forward, a new one will be opened.
DanutGavrus commentedon Aug 14, 2023
Hi, @jeet-dhandha and @jliexpensify! On smaller devices, we have a problem, participant details are not appearing at all, so you don't have the option to scroll:
smaller_devices.mp4
The bug was reported here on Slack, but the reporter stated to use web zoom as he didn't have a small device. We are using
ScrollViewsfor our pages, thus content on smaller devices becomes scrollable(Share code page from Setting is an example), but theScrollViewis missing from the Request money page. We also have: "If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page." in the contributing guides.As stated by @jeet-dhandha, it is intentional to scroll the participants list, and I think we can achieve that by wrapping the Money request page in a
ScrollView, and then, setting a max height for the current participantsScrollViewto the total of showing 3 participants before having to scroll.Do you think this is a valid bug? Should we reopen this one and fix it here, should we open that reported bug from Slack and fix it there, or is it not a bug? Thank you both for your time!
DanutGavrus commentedon Aug 14, 2023
In case this one gets reopened, I'm leaving a potential Proposal here, otherwise, just ignore this comment. Thanks!
Proposal
Please re-state the problem that we are trying to solve in this issue.
Participants detail are hidden at all after clicking on show more button on smaller devices.
What is the root cause of that problem?
In the
MoneyRequestConfirmPagewe are not using theScrollViewcomponent as we do in other places such as in theShareCodePagewhere we leave the header at the top, and wrap the content which should be scrollable.What changes do you think we should make in order to solve the problem?
ScrollViewinsideMoneyRequestConfirmPage, wrapping theMoneyRequestConfirmationList, and leaving the header stay at the top;ScrollViewis also missing from theMoneyRequestAmountForm, the page where we input the amount on our custom numpad, making it possible for the Next button to be missing, so we may add it there too;ScrollView's, if any;ScrollViewsin order to prevent similar future bugs from happening. I think this will require some time & attention though.jliexpensify commentedon Aug 16, 2023
Original issue - @mallenexpensify what do you think? Should we consider this one as a bug?
@DanutGavrus just curious, what is defined as a
smaller device?DanutGavrus commentedon Aug 16, 2023
The above video has a resolution of
930px width × 588px heightfor example.41 remaining items
melvin-bot commentedon Sep 1, 2023
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:
jliexpensify commentedon Sep 6, 2023
Payment summary:
Upwork job
Is that correct?
@eVoloshchak please complete the checklist! @DanutGavrus please accept the offer.
eVoloshchak commentedon Sep 10, 2023
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the pageIs it easy to test for this bug? Yes
Is the bug related to an important user flow? Yes
Is it an impactful bug? No
This is a visual bug that can be reproduced on a small number of devices (it has to be a fairly small device/window) that doesn't prevent the user from continuing to use the app. I think the checklist item is enough, the problem won't occur for all of the newly created pages, I don't think a regression test is needed here
eVoloshchak commentedon Sep 10, 2023
Looks like has introduced a regression in #26544
This was quickly resolved by a follow-up PR from @DanutGavrus, it was deployed to production 4 days ago, so the payment date isn't due just yet (payments will be issued 7 days after all regressions are fixed (ie: deployed to production), source)
DanutGavrus commentedon Sep 10, 2023
We also have this issue #26418 which was not opened to payment and was mentioned in my follow up PR. In the dupe issue, which was opened to external contributors but was fixed by me, we are going to only pay the reporter. You can see in my follow-up PR changes that I only forgot to add 1 prop to a ScrollView and caused a very small issue(the Next button's position was not docked to bottom anymore, but everything was still functional). The original PR was a little bit tricky(as you can see with the double ScrollView) and my focus was on the important things(as you can see from the attached videos and the variety of tests) and I've also voluntarily fixed another issue in advance which was not reported yet(added a ScrollView for our custom keyboard page too). IMO, the button position seems a very small detail which could easily be omitted by anyone. Those being said, I would like to ask if this small detail may not be considered a regression for the previously mentioned things, and if we may continue with the payment as is. Thanks!
jliexpensify commentedon Sep 10, 2023
Thanks for the context @DanutGavrus - since I am not an Engineer, I will ask @Gonals to step in and evaluate your comment and make the final call.
If he agrees with this comment:
And we choose to not count this as a regression, then the payouts in this comment would still be valid. If it is a regression, then we should apply a 50% penalty (i.e. $750 all up).
jliexpensify commentedon Sep 14, 2023
Heya @Gonals - can I get your thoughts here?
p.s. After tomorrow, it'll be 7 days after deployment of the new PR.
Gonals commentedon Sep 15, 2023
I think that is a fair assessment. It is a small issue in a moderately complex PR. I think, in this particular case, we could go ahead with payment as usual.
jliexpensify commentedon Sep 15, 2023
Great, thanks Alberto. Paying out as per this comment as the second PR has been deployed for 7 days.
jliexpensify commentedon Sep 15, 2023
Paid and job closed, cheers!
JmillsExpensify commentedon Sep 21, 2023
$1,500 payment approved for @eVoloshchak based on BZ summary.