-
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
fix: set initial task title and description values from Onyx if present #19908
Conversation
@aldo-expensify @abdulrahuman5196 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] |
@abdulrahuman5196 A note regarding clearing the values when the "x" button is clicked: It looks like this is already working as expected. Clicking on the close button and then selecting Assign task to create a new task opens the form with empty fields (when the title and description have been previously filled), so there is no need to implement this here. |
@akinwale but when we open the task page directly via url after previously filling details in assign page and closing, we will be showing the old data. |
I don't think we need to do anything in this case in my opinion. If the user accidentally closes or refreshes the page, it's a good experience for the input data to be preserved. |
@@ -80,13 +98,18 @@ const NewTaskPage = (props) => { | |||
ref={(el) => (inputRef.current = el)} | |||
inputID="taskTitle" | |||
label={props.translate('newTaskPage.title')} | |||
defaultValue={props.task.title} |
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.
Isn't defaultValue reduntant here? Can we remove it if so?
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.
This is correct. Will remove it.
@akinwale I wasn't mentioning about tab close or refresh. I meant to clear when user manually closed the drawer with the close button. I don't think we retain old values user manually presses the header x button @aldo-expensify Correct me if wrong. Untitled.45.mp4 |
@abdulrahuman5196 Ah, I see what you mean. The task info is actually being cleared out when the Assign task menu item is clicked. This is happening here:
To clear out the information when the close button is clicked / pressed, then I'll need to add a call to |
@@ -80,13 +98,18 @@ const NewTaskPage = (props) => { | |||
ref={(el) => (inputRef.current = el)} | |||
inputID="taskTitle" | |||
label={props.translate('newTaskPage.title')} | |||
defaultValue={props.task.title} |
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.
@@ -70,7 +70,10 @@ const NewTaskDescriptionPage = (props) => { | |||
> | |||
<HeaderWithCloseButton | |||
title={props.translate('newTaskPage.description')} | |||
onCloseButtonPress={() => Navigation.dismissModal()} | |||
onCloseButtonPress={() => { | |||
Navigation.dismissModal(); |
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.
Navigation.dismissModal() and TaskUtils.clearOutTaskInfo() is repeated at multiple places.
Can we create a util function like this?
Lines 479 to 483 in 465b788
function clearOutTaskInfoAndNavigate(reportID) { | |
clearOutTaskInfo(); | |
setParentReportID(reportID); | |
Navigation.navigate(ROUTES.NEW_TASK_DETAILS); | |
} |
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.
/> | ||
</View> | ||
<View style={styles.mb5}> | ||
<TextInput | ||
inputID="taskDescription" | ||
defaultValue="" | ||
defaultValue={props.task.description} |
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.
Same as the previous comment. DefaultValue is redundant here @akinwale
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.
@abdulrahuman5196 Done.
if (taskDescription !== props.task.description) { | ||
setTaskDescription(props.task.description); | ||
} | ||
}, [props.task, taskTitle, taskDescription]); |
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.
I think have taskTitle, taskDescription here causing the below issue
I am unable to update the title value when coming back to new task page.
Untitled.2.mp4
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.
I have removed the comparison from the useEffect
which was causing this. The useEffect
will now run only once when the component mounts, since the only dependency is props.task
which is coming from Onyx.
const [taskDescription, setTaskDescription] = useState(props.task.description); | ||
|
||
useEffect(() => { | ||
if (taskTitle !== props.task.title) { |
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.
Do we need to 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.
We don't need this since the useEffect
call will run only once when the component mounts.
@akinwale Kindly check on the above issues |
@abdulrahuman5196 Thanks for the review. I have pushed a new commit with the necessary fixes. |
@@ -63,7 +77,7 @@ const NewTaskPage = (props) => { | |||
> | |||
<HeaderWithCloseButton | |||
title={props.translate('newTaskPage.assignTask')} | |||
onCloseButtonPress={() => Navigation.dismissModal()} | |||
onCloseButtonPress={() => TaskUtils.dismissModalAndClearOutTaskInfo()} | |||
shouldShowBackButton | |||
onBackButtonPress={() => Navigation.goBack()} |
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.
@akinwale On this page alone, kindly clear out the task TaskUtils.dismissModalAndClearOutTaskInfo()
since back action here closes the modal and has to reset the onyx data.
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.
@abdulrahuman5196 Done.
Reviewer Checklist
Screenshots/VideosWebUntitled.6.mp4Mobile Web - ChromeUntitled.3.mp4Mobile Web - SafariUntitled.7.mp4DesktopUntitled.4.mp4iOSUntitled.5.mp4AndroidUntitled.10.mp4 |
@akinwale Offline test should |
@akinwale Kindly fix these issues
Untitled.9.mp4 |
|
@abdulrahuman5196 Done. See comments here. |
Is form setting the value as undefined for not passing default value or since If later we should update the useState() to not set undefined on |
I think the latter mentioned is the issue here #19908 (comment). Correct me if wrong. @akinwale |
@abdulrahuman5196 I believe it's the latter where props.task.description is undefined with useState. What do you suggest here? Set useState('') for both taskTitle and taskDescription, or just taskDescription alone? |
and similar way inside useEffect.(both should be only for description since task is not expected to have a default value) This should be best way since we handle value and onValueChange.
|
@abdulrahuman5196 Thanks for the update. All done. Tested and it works properly. |
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 looks good and works well. Reviewers checklist is also complete here - #19908 (comment)
All yours @aldo-expensify
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
✋ 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/aldo-expensify in version: 1.3.24-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀
|
Details
Set the the task title and description on the initial page in the assign task flow using values from Onyx if they are present.
Fixed Issues
$ #19460
PROPOSAL: #19460 (comment)
Tests
Offline tests
Same as tests.
QA Steps
Same as tests.
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
19460-web.mp4
Mobile Web - Chrome
19460-android-chrome.webm
Mobile Web - Safari
19460-ios-safari.mp4
Desktop
19460-desktop.mp4
iOS
19460-ios-native.mp4
Android
19460-android-native.webm