-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
show individual split amount on Bill Split IOU Previews #18715
show individual split amount on Bill Split IOU Previews #18715
Conversation
@Santhosh-Sellavel @PauloGasparSv One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@Julesssss @mountiny ready for review 🚀 |
I believe this will happen on main too Not sure why job1 fails. Can you please re-run? |
From logs it seems like the chat report id of the splits is not correct, would begreat if @Julesssss could try too to see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looking good. As for the error, I saw this briefly myself yesterday after merging main, but I don't think it's related to these changes.
Here's a separate issue for the error |
All yours @Santhosh-Sellavel. Signing out/in should resolve the issue seen above 🤞 |
…ssue-18706-split-amount-iou-preview
@0xmiroslav Seems we ran into conflicts again. |
…706-split-amount-iou-preview
Fixed! |
Cool, thanks will review shortly |
Reviewer Checklist
Screenshots/Videos |
Should we show splits only for the last request? @mountiny or @Julesssss |
As far as I see, I have no problem creating the split in new group chats, but it happens for old group chats. |
Ah. then this is a separate bug I think |
Sorry @0xmiroslav We have conflicts again, please resolve them thanks. |
…ssue-18706-split-amount-iou-preview
onPressHandler(event); | ||
}, | ||
[onPressHandler], | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file change is just to fix prettier diff so you can ignore
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is more towards UI which looks good, tests well approving!
cc: @mountiny @Julesssss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you both @0xmiroslav @Santhosh-Sellavel
|
@mountiny we can merge this. @Julesssss already approved before merge conflict |
@0xmiroslav just to confrim I am not sure if all the code necessary in Staging? Dont want to get merge conflicts wiht the CP |
I think it's fine to remove |
Cool agreed, thanks! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.14-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
Details
show individual split amount on Bill Split IOU Previews
Fixed Issues
$ #18706
PROPOSAL: Coming from https://expensify.slack.com/archives/C02NK2DQWUX/p1683711639585659
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android