-
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
feat: use MenuItem instead of TaskSelectorLink on Task page #20145
Conversation
@thienlnam @s77rt 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] |
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.
src/components/MenuItem.js
Outdated
const stylesWithoutPadding = _.isArray(props.style) | ||
? _.map(props.style, (style) => _.omit(style, ['padding', 'paddingVertical', 'paddingHorizontal'])) | ||
: _.omit(props.style, ['padding', 'paddingVertical', 'paddingHorizontal']); | ||
const paddingStyles = _.isArray(props.style) | ||
? _.map(props.style, (style) => _.pick(style, ['padding', 'paddingVertical', 'paddingHorizontal'])) | ||
: _.pick(props.style, ['padding', 'paddingVertical', 'paddingHorizontal']); |
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.
Can you please get rid of this? I don't see the need for a new View (or new padding styles)
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.
@s77rt This is needed to create the below layout:
If we remove the View
and use it as it is right now, the label
will just appear to the left of icon since the container is flex-row
.
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.
In this case we should just change the default style to flex column (add it below props.style) and in the new view use flex row.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Actually, It won't require a revamp. Ignore my previous comment please.
Fixed the spacing issue. I can't seem to reproduce it after merging to main. It might be that my branch was too old. |
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.
src/pages/tasks/NewTaskPage.js
Outdated
icon={assignee.icons} | ||
onPress={() => Navigation.navigate(ROUTES.NEW_TASK_ASSIGNEE)} | ||
shouldShowRightIcon | ||
style={styles.popoverMenuItem} |
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.
style={styles.popoverMenuItem} |
Not required
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.
Handled.
@s77rt Updated screenshots and storybook. |
src/pages/tasks/NewTaskPage.js
Outdated
<MenuItemWithTopDescription | ||
<MenuItem |
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.
Why we are using MenuItem and not MenuItemWithTopDescription
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.
@s77rt This is needed in order to show the title in bold. This is how it was done at other places.
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.
Right right
Co-authored-by: Abdelhafidh Belalia <16493223+s77rt@users.noreply.github.com>
After creating a task getting these errors:
|
Hm... that's weird. I'll check this. |
Updated. |
@@ -153,15 +153,15 @@ const NewTaskPage = (props) => { | |||
<MenuItem | |||
label={assignee.displayName ? props.translate('newTaskPage.assignee') : ''} | |||
title={assignee.displayName || ''} | |||
description={assignee.subtitle ? assignee.subtitle : props.translate('newTaskPage.assignee')} | |||
description={assignee.displayName ? assignee.subtitle : props.translate('newTaskPage.assignee')} |
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 does not make sense. Did you mean to set the description to assignee.displayName
(currently it's still assignee.subtitle
)
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.
No. The check (to show description or the label) should depend on the displayName like everywhere else.
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.
Currently the email is not shown. Can you please add that
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 do you mean? Can you share a screenshot of what you're seeing?
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.
Hm... I think this happens on staging as well. Do we want to fix this here?
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.
yeah let's fix this here assuming it's not much trouble
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.
@s77rt I've checked it and I think we're doing it on purpose. The subtitle is being fetched using the getChatRoomSubtitle
function in the ReportUtils. Specifically, the following condition returns it:
if (!isDefaultRoom(report) && !isUserCreatedPolicyRoom(report) && !isPolicyExpenseChat(report)) {
return '';
}
I'm not sure if this is really a bug and we should block this PR because of that.
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 on main already so I won't block on that.
Reviewer Checklist
Screenshots/Videos |
@s77rt Fixed. |
@allroundexperts Can you please also cover the case where the menu item is clicked |
@s77rt I've fixed it. But it might be worth checking with @shawnborton about the border colours of multiple avatar component for the menu item above in all three states (not hovered, hovered, pressed) |
Normal State ![]() Hovered State ![]() Pressed State ![]() @shawnborton Please let us know if these need changes. |
That makes sense to me. |
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! 🚀
cc @thienlnam
Awesome, thanks guys! |
✋ 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/thienlnam in version: 1.3.27-0 🚀
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.27-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.27-7 🚀
|
onPress={() => Navigation.navigate(ROUTES.NEW_TASK_SHARE_DESTINATION)} | ||
label="newTaskPage.shareSomewhere" | ||
isShareDestination | ||
disabled={Boolean(props.task.parentReportID)} |
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 condition was not considered while making the change causing this regression.
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.
Sorry for that. Thanks for the highlight @sobitneupane
<MenuItem | ||
label={assignee.displayName ? props.translate('newTaskPage.assignee') : ''} | ||
title={assignee.displayName || ''} | ||
description={assignee.displayName ? assignee.subtitle : props.translate('newTaskPage.assignee')} |
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.
Hi, this caused a bug - Phone number contact in task assignee is displayed with @expensify.sms
We should have sanitized the asignee because it can be a phone number.
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.
@rushatgabhane Thanks for the report. Just trying to understand if this is something we missed because I think we just mapped the logic from TaskSelectorLink
where the assignee was handled as is without taking phone number in consideration
App/src/components/TaskSelectorLink.js
Lines 83 to 89 in 49895f4
{props.alternateText ? ( | |
<Text | |
style={alternateTextStyle} | |
numberOfLines={1} | |
> | |
{props.alternateText} | |
</Text> |
StyleUtils.getBackgroundAndBorderStyle(themeColors.sidebar), | ||
pressed ? StyleUtils.getBackgroundAndBorderStyle(themeColors.buttonPressedBG) : undefined, | ||
hovered && !pressed ? StyleUtils.getBackgroundAndBorderStyle(themeColors.border) : 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.
This regression was probably caused by the PR.
Issue: Task - Second avatar of group avatar is highlighted when long pressing on Share somewhere
Details
This PR removes
TaskSelectorLink
component and replaces it with already existingMenuItem
in the NewTask page.Fixed Issues
$ #20007
PROPOSAL: #20007 (comment)
Tests
+
icon in the LHN and selectNew Task
option.Verify that the
Task Confirmation
page has menu items which are similar to what we see on user profile page.Offline tests
N/A
QA Steps
+
icon in the LHN and selectNew Task
option.Verify that the
Task Confirmation
page has menu items which are similar to what we see on user profile page.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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