-
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] Implement the SubScript avatar for workspace money requests #18085
Conversation
@aimane-chnaif @deetergp 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] |
Code looks good and tests well on web. |
reviewing shortly |
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.
LGTM and tests well! Thanks for the quick work!
@aimane-chnaif the subscript avatar should only show when requesting money, but that functionality is not yet available so we are hardcoding test steps. As for the tooltip, I agree that we should add it for the workspace avatar |
ok so you mean that request money won't available at all in policy expense 1:1 chat? Line 1711 in a786349
|
It will be, but the feature is not ready yet. |
Regarding the tooltip, I can add it, but we weren't using tooltip on the normal |
As
This is how it's done in OptionRowLHN: (MultipleAvatars has Tooltip>Avatar structure) App/src/components/LHNOptionsList/OptionRowLHN.js Lines 140 to 165 in 9b7d6cd
|
Right, Thanks. I'm saying, we should NOT add tooltips because we aren't displaying tooltips here in the first place. |
OK, I totally missed that we ARE using tooltips now (I missed the |
OK, updated with tooltips: 2023-05-01_14-16-14.mp4 |
<SubscriptAvatar | ||
mainAvatar={{source: avatarSource, type: CONST.ICON_TYPE_AVATAR}} | ||
secondaryAvatar={ReportUtils.getIcons(props.report, {})[0]} | ||
mainTooltip={props.report.ownerEmail} |
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.
mainTooltip={props.report.ownerEmail} | |
mainTooltip={actorEmail} |
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.
Agree that actorEmail is more accurate here (although they might overlap often)
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.
Confirmed from slack discussion that current logic we have in the PR is correct.
🎯 @aimane-chnaif, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #18260. |
checklisting shortly |
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.
One question from me though is that we will also have the transaction edited
report action which afaik is also IOU action type, same as deleted
are we fine with whose showing the subscript too? I assume yes, but just wanted to ask that.
<SubscriptAvatar | ||
mainAvatar={{source: avatarSource, type: CONST.ICON_TYPE_AVATAR}} | ||
secondaryAvatar={ReportUtils.getIcons(props.report, {})[0]} | ||
mainTooltip={props.report.ownerEmail} |
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.
Agree that actorEmail is more accurate here (although they might overlap often)
Reviewer Checklist
Screenshots/VideosWebweb.movDesktopdesktop.mov |
✋ 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/luacmartins in version: 1.3.9-12 🚀
|
shouldShowSubscriptAvatar={this.props.shouldShowSubscriptAvatar} | ||
report={this.props.report} |
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 make sure to pass these values in ReportTransaction to show subscript avatar on IOUDetailsModal after we implement money request in workspace chat
App/src/components/ReportTransaction.js
Lines 65 to 68 in d120f6a
<ReportActionItemSingle | |
action={this.props.action} | |
wrapperStyles={[styles.reportTransactionWrapper]} | |
> |
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.
@aimane-chnaif can you submit a screenshot of where you think the SubscriptAvatar should be displayed as well? Just trying to keep things consistent since a lot of these screens will be updated/removed.
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.
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.
Yea, I think that page will be removed since now we'll show the report in ReportScreen and render the reportActions for each of them.
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
Sorry, I did a refactoring that ended up being much larger than I had intended when I moved the policy collection to be inside of
ReportUtils
. That's really going to speed up all those components that no longer are connected to the collection though.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/278889
Tests
ReportActionsList.js
to setshouldShowSubscriptAvatar={true}
Offline tests
None
QA Steps
None. This can't be QA'ed yet because there is no way to request money from a workspace at this point.
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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android