-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Deselect attendees when splitting bill #3881
Deselect attendees when splitting bill #3881
Conversation
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 looks really good 👍
Great work so far, and I'm eager to discuss this solution with you further in the comments.
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 is looking great! I know we've done several rounds of reviews, but I think it's really gonna show in the end. Thanks for hanging in there!
Actually, thanks for your time! |
OK @rushatgabhane, go ahead and test this branch on Android, Web, and Mobile Web. I'll test Desktop and iOS on your behalf. Though, I do highly recommend you set up a mac vm to test iOS changes in the future. The chance of us hiring contributors who do not have an iOS testing environment in the future will continue to decrease. |
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.
Just passing through to leave some quick style notes.
Yes, thank you! |
I've tested this branch. And updated the screenshots for web, android, and mWeb. |
OK! Just tested on iOS and desktop. Lookin' good. 👍 |
It looks like there are now merge conflicts here. I've approved the code here, and I'll be out of office next week, so perhaps @marcaaron can give the final approve and merge once the conflicts are addressed. |
Apologies for the forced-push! I had messed up the commit history. Merge conflicts are fixed and tested ✔ |
✋ 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 in version: 1.0.82-8🚀
|
🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀
|
Progress so far.
Details
Comment
To check if a split happening from a group DM,
props.route.params.reportID
is checked.Fixed Issues
$ #3841
Tests / QA
Tested On
Don't have a Mac to test iOS, Desktop.
Screenshots
Web
newWebVideo.mp4
Mobile Web
Desktop
iOS
Android
newAndroid.mp4