-
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
[No QA] Skip IOU participants step if launched from within chat #2321
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.
Changes look good! I also went ahead and added the imports to the description
👋 Just a heads up that IOUConfirm #2120 has recently merged 👍 |
# Conflicts: # src/pages/iou/IOUModal.js
login: personalDetails.login, | ||
text: personalDetails.displayName, | ||
alternateText: personalDetails.login, | ||
icons: [personalDetails.avatar], |
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 should now be an avatarThumbnail on personalDetails which is an 80x80 version of the image, can we double check to make sure we're using those instead of full sized? You might also need to update getPersonalDetailsForLogins
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.
Good spot @thienlnam.
I could be wrong, but I think that might be out of scope for this issue as participant Avatars were implemented in a previous issue. If we spot that the incorrect size is used though, I'll be happy to create a new issue for this.
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.
That's fair, let's go ahead with that then if the incorrect sizes are showing up
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.
Noticed a couple of small things
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.
Ah, actually could you keep the new dynamic routes, please. We'll use both the old static and new dynamic roites.
🚀 Deployed to staging 🚀
|
@Julesssss / @thienlnam This one doesn't look QAable by us. Can you let me know if this is internal QA or if there are QA steps that can be executed by Applause? Thanks in advance |
Hey @isagoico, our bad - looks like the routes for these options to be shown aren't actually enabled yet so this is not something that can be tested yet |
🚀 Deployed to production in version: 1.0.39-5🚀
|
Details
IOUParticipants
if routed from a chatFixed Issues
Fixes #2246
Tests
src/pages/home/report/ReportActionCompose.js
:src/pages/home/report/ReportActionCompose.js
:src/ROUTES.js
:this.props.reportID
from the navigation request above and go through the flow again, this time confirming that the IOUParticipants step does appear.QA Steps
Same as above.
Tested On
Screenshots
Web
Screen.Recording.2021-04-20.at.1.01.57.AM.mov
Mobile Web
Screen.Recording.2021-04-20.at.1.01.08.AM.mov
Desktop
Screen.Recording.2021-04-20.at.1.04.32.AM.mov
iOS
Screen.Recording.2021-04-20.at.12.54.35.AM.mov
Android
Screen.Recording.2021-04-20.at.1.16.12.AM.mov