-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 Task FrontEnd Changes #17992
Create Task FrontEnd Changes #17992
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
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.
It's really coming together 🎉
title: 'Título', | ||
description: 'Descripción', | ||
shareIn: 'Compartir en', | ||
pleaseEnterTaskName: 'Por favor introduce un título', | ||
pleaseEnterTaskAssignee: 'Por favor, asigna una persona a esta tarea', |
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.
Reminder for me to get translations for this, we have some certain terminology we are trying to keep consistent
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.
@thienlnam I posted in open-source to get someone to check translations. Someone asked for the En/Es versions and I provided them but I haven't heard anything else about it.
0e1bd1f
to
3560b3a
Compare
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.
Left this initial comment first, but taking a look at the onyx data next
Optimistically: |
I have read the CLA Document and I hereby sign the CLA |
3285c82
to
f166357
Compare
f166357
to
7e1df15
Compare
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.
Couple parameters to update to match with backend
src/libs/actions/Task.js
Outdated
|
||
// Grab the assigneeChatReportID if there is an assignee and if it's not the same as the parentReportID | ||
// then we create an optimistic add comment report action on the assignee's chat to notify them of the task | ||
const assigneeChatReportID = ReportUtils.getChatByParticipants([assignee]).reportID; |
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.
Nice catch! We want to leave the option for a task to be created without an assignee, only title and share destination are required. I've changed to logic to check to make sure there is an assignee before fetching the DM chat report with that assignee.
417e60f
to
ee50e02
Compare
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.
CreateTask
command is still failing, there is some missing parameters
if (option.alternateText) { | ||
// Clear out the state value, set the assignee and navigate back to the NewTaskPage | ||
setSearchValue(''); | ||
TaskUtils.setAssigneeValue(option.alternateText, props.task.shareDestination); |
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.
Coming from #18649:
We should have used option.login
instead of option.alternateText
for phone number accounts.
Here's the root cause: #18649 (comment)
This issue was not addressed in this PR. We accepted white spaces as task description. |
This issue was not address in this PR. Issue: Error appears for a second when user select new member as Assignee |
disabled: false, | ||
}; | ||
|
||
const TaskSelectorLink = (props) => { |
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 component was not clipping text appropriately for longer text which caused #18659
{ | ||
onyxMethod: Onyx.METHOD.SET, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticTaskReport.reportID}`, | ||
value: optimisticTaskReport, |
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.
Coming from #19134:
This should always be added in optimisticData when build new report, so that they show with opacity when created offline.
pendingFields: {
createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},
// the response | ||
function onSubmit() { | ||
if (!props.task.title || !props.task.shareDestination) { | ||
setSubmitError(true); |
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.
Coming from Assign Task - An incorrect error message is displayed when clicking confirm task:
This was overlooked. As confirmError
error is set as initial state value, Please enter a title and select a share destination
message is always displayed even though title is already filled.
} | ||
|
||
// Create the CreatedReportAction on the task | ||
const optimisticTaskCreatedAction = ReportUtils.buildOptimisticCreatedReportAction(optimisticTaskReport.reportID); |
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.
Fyi looks like we probably meant to pass ownerEmail
in the param, not reportID :D
Coming from #21361. We were getting error after adding the title and moving to the next step. Info: #21361 (comment) |
<Form | ||
formID={ONYXKEYS.FORMS.NEW_TASK_FORM} | ||
submitButtonText={props.translate('common.next')} | ||
style={[styles.mh5, styles.mt5, styles.flexGrow1]} |
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.
Coming from #21754, We have extra padding (styles.mt5
) in all create task forms.
Coming from here #22060. We missed to clear the pending action onyx state after API success #22060 (comment) |
onBackButtonPress={() => Navigation.goBack()} | ||
/> | ||
<View style={[styles.flex1, styles.w100, styles.pRelative]}> | ||
<OptionsSelector |
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 all, this PR caused issue #20075
The asignee selection list can take a long time to filter. So we should have debounced search when implementing OptionsSelector (like done in this PR)
This case was missed while implementing New Task Page. Issue: Web - UserB can still create a task in room chat when UserA changes post permission to "Admins only" |
}); | ||
} | ||
|
||
const successData = []; |
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 caused a regression. The successData
array is empty but we should be clearing the pendingActions. (Coming from #22907)
This issue was not handled in this PR. Issue: Assign Task - Workspace avatar shows 'U' |
} | ||
|
||
// Create the CreatedReportAction on the task | ||
const optimisticTaskCreatedAction = ReportUtils.buildOptimisticCreatedReportAction(optimisticTaskReport.reportID); |
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.
Coming from No activity yet appears in task item of search RHN.
We should have updated getLastMessageTextForReport()
to show the task details in LHN description.
Details
This sets up the UI and the necessary actions in order to Create a Task.
Fixed Issues
$ #16855
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
NOTE: The CreateTask AuthCommand is not currently deployed, so the online flow will not work as expected. However, the offline flow should work.
Assignee and Share Destination are Different
Assignee and Share Destination are the Same
Offline tests
Same as Online Tests
QA Steps
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
Lower.Res.Web.Test.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
Low.Res.Desktop.Version.mov
iOS
Low.Res.IOS.Test.mov
Android
Low.Res.Android.Video.mov