-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Create IOU participant selection step. #1738
Create IOU participant selection step. #1738
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
return ( | ||
<View style={[styles.flex1, styles.w100]}> | ||
<Text style={[styles.headerText, styles.pt3, styles.ph5]}> | ||
TO |
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.
Let's just make this To
for now and use the style styles.formLabel
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! will implement the change
Hey @barun1997, thanks for the submission! Unfortunately I completely missed this today, but look forward to reviewing first thing Monday. |
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 on my initial read, here are some style changes I'd recommend
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js
Outdated
Show resolved
Hide resolved
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsSplit.js
Outdated
Show resolved
Hide resolved
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsSplit.js
Outdated
Show resolved
Hide resolved
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.
Didn't get all the way through this, but had just a few comments.
personalDetails: PropTypes.objectOf(personalDetailsPropTypes).isRequired, | ||
|
||
// All reports shared with the user | ||
reports: PropTypes.shape({ |
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.
Should this be PropTypes.arrayOf(PropTypes.shape({...}))
?
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.
@roryabraham The prop (props.reports), in this case, is an object with the given shape. The getNewGroupOptions
function maps it into an array.
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js
Outdated
Show resolved
Hide resolved
Thank you @roryabraham for the comments. I will look into it and resolve the issues. |
recheck |
Hey @Julesssss, before the weekend, the file |
cc @marcaaron I think this might be related to the |
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js
Outdated
Show resolved
Hide resolved
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js
Outdated
Show resolved
Hide resolved
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js
Outdated
Show resolved
Hide resolved
src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsSplit.js
Outdated
Show resolved
Hide resolved
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.
Looking good!
The only other change I'd like suggest is that we keep the isLoading
logic. The idea is that we'll reuse the IOU {loading}
Onyx key for each page. Setting it to true via an Onyx Action. Please let me know if you have any questions or concerns about this.
8905fe2
to
0edbd6c
Compare
@Julesssss I re-implemented the loading logic. Please kindly take a look into it, and let me know if you have any suggestions. |
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.
Thanks for addressing the feedback, I think there are a few more comments to work through.
As a final change, would it be possible to pass the participants back to IOUModal every time the selection changes, rather than on the button press? Then pass them back down to IOUParticipantsRequest|IOUParticipantsSplit
via a participants prop. This would allow us to remember the selection if the user navigates back to the amount page from the participants selection.
Sorry for not making this clear in the initial issue!
@Julesssss Sure, no worries! I will implement the change first thing tomorrow. |
Hi @Julesssss, I implemented the change as you requested. But I am a bit not sure how the UI for If we don't have to show a tick mark beside the selected participant, in that case, perhaps it's not necessary to pass the participants prop onto |
0edbd6c
to
b27309a
Compare
Good point, I agree that we should not pass the prop into |
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.
The rest of the PR looks good to me. Happy to approve once you remove the prop from IOUParticipantsRequest
and add the missing comment.
@roryabraham there are some responses to your original comments, would you mind reviewing them? |
e3e5087
to
f085f11
Compare
Changes have been made to address your comments
Looks great! 👍 |
|
||
if (this.state.userToInvite) { | ||
sections.push(({ | ||
undefined, |
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.
What's this doing here? :D
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.
:ohnothing:
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.
Perhaps this was how we were hiding the title back then?
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.
Perhaps? Idk why we'd want a property undefined
set to undefined
though 😅
Fixes: #1694
<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>
Details
Tests
Next
button to appear or the request amount page is well rendered.Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android