feat(1263): Add purpose & status to task card, based on task card in website-my #1319
+269
−27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Saitharun279 is attempting to deploy a commit to the RDS-Team Team on Vercel. A member of the Team first needs to authorize it. |
Saitharun279
changed the title
Saitharun279
changed the title
tejaskh3
requested changes
Jan 17, 2025
|
There was a problem hiding this comment.
Hello @Saitharun279, could you please check if we can use predefined color variables from variables.scss instead of adding random colors?
| Purpose:{' '} | ||
| </b> | ||
| <span className={styles.cardPurposeText}> | ||
| {editedTaskDetails.purpose} |
There was a problem hiding this comment.
do we have purpose field when we create the TCR from status site?
There was a problem hiding this comment.
I just checked, it is not present in the TCR.
I think it needs to be added.
| @@ -91,6 +91,16 @@ | |||
| color: #aeaeae; | |||
| } | |||
|
|
|||
| .cardPurposeAndStatusFont { | |||
| font-size: 1.1rem; | |||
| color: #555; | |||
There was a problem hiding this comment.
Can we use a predefined color from variables.scss here?
There was a problem hiding this comment.
All the colors that I used are not present in variables.css. I have taken them from my site.
should i add them in variables.scss ?
|
Attached mobile video |
yesyash
requested changes
Jan 18, 2025
| .taskStatusEditMode { | ||
| margin-top: 0.8rem; | ||
| } | ||
|
|
||
| .taskStatusUpdate { | ||
| border: 1px solid #000000b3; |
There was a problem hiding this comment.
- are there existing css variables we can use instead of hardcoding the color?
There was a problem hiding this comment.
Didn't find any existing css variables for this one.
src/components/tasks/card/index.tsx
Outdated
| @@ -266,6 +266,10 @@ const Card: FC<CardProps> = ({ | |||
| setIsEditMode(true); | |||
| }; | |||
| const isEditable = shouldEdit && isUserAuthorized && isEditMode; | |||
| const isSelfTask = editedTaskDetails.assignee === data?.username; | |||
| const verifiedTask = | |||
There was a problem hiding this comment.
- should this be named
isTaskCompletedas we're doing a check on the percentage this task has be completed?
There was a problem hiding this comment.
- The dropdown to update task staus should only be shown for non verified tasks, this variable
verifiedTaskis to check if task status is verified. - The check for 100 percent completion can be removed, as the status updation API allows update to Verified when percentage is 100.
I will remove this percentage check in next commit.
I think the variable can be renamed to isVerifiedTask.
There was a problem hiding this comment.
Removed 100 percentage check and modified variable name to isVerifiedTask
| }: Props) => { | ||
| const [saveStatus, setSaveStatus] = useState(''); | ||
| const [updateTask] = useUpdateTaskMutation(); | ||
| const [updateSelfTask] = useUpdateSelfTaskMutation(); | ||
|
|
||
| const onChangeUpdateTaskStatus = ({ |
There was a problem hiding this comment.
- can we update this function to use try catch to make this cleaner and easier to update? example:
const onChangeUpdateTaskStatus = async ({
newStatus,
newProgress,
}: taskStatusUpdateHandleProp) => {
setSaveStatus(PENDING);
const payload: { status: string; percentCompleted?: number } = {
status: newStatus,
};
if (newProgress !== undefined) {
payload.percentCompleted = newProgress;
}
setEditedTaskDetails((prev: CardTaskDetails) => ({
...prev,
...payload,
}));
try{
if (isDevMode && isSelfTask){
await updateSelfTask({ id: task.id, task: payload })
} else {
await updateTask({ id: task.id, task: payload });
}
} catch(error){
setSaveStatus(ERROR_STATUS);
} finally {
setTimeout(() => {
setSaveStatus('');
}, 3000);
}
};- should we call
setEditedTaskDetailsafter the API call is successful preventing any inconsistent states? - can we remove the
setTimeoutfrom thefinallyblock?
There was a problem hiding this comment.
Will update in next commit.
But not sure about removing setTimeout, will check and update that as well.
|
Saitharun279
changed the title
Saitharun279
changed the title
Saitharun279
changed the title
Saitharun279
changed the title
Saitharun279
changed the title
…h and toast messages.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Date: 17 January, 2025
Developer Name: Saitharun B
Issue Ticket Number
Description
Added code changes to let task owner do the following:
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Web
local_testing.mov
mobile
local.testing.moble.mov
Test Coverage
Screenshot
Additional Notes
design doc link