-
Notifications
You must be signed in to change notification settings - Fork 167
bug-fix: #225 re-design task card #1134
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
base: develop
Are you sure you want to change the base?
Conversation
|
@palakgupta2712 is attempting to deploy a commit to the RDS-Team Team on Vercel. A member of the Team first needs to authorize it. |
| $orange: #ffa500; | ||
| $light-red-1: #ffb0b0; | ||
| $light-red-2: #ffdddd; | ||
| $light-red-2: #fef2f2; |
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 above
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 is same as above? have deleted the hex and added a new.
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.
Might affect other than task card items?
If not we can ignore it
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.
Please try not adding comments like Same as above, it confuses the Author. If you want to reference add comments beforehand then copy the link of that comments which are repeated and add comment links in the next ones
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.
Also be specific to what changes are you referring to also helps
src/components/tasks/card/index.tsx
Outdated
| isDevMode={isDevMode} | ||
| /> | ||
| ) : ( | ||
| <div className={styles.statusContainer} style={{}}> |
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.
cleanup
| <p | ||
| data-testid="task-status" | ||
| className={`${styles.statusText} ${ | ||
| 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.
Should be moved out for better readability
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.
out as in?
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.
NIT: we can keep it in a variable outside jsx for some readability
| data-testid="task-status" | ||
| className={`${styles.statusText} ${ | ||
| styles[ | ||
| `statusText${handleStatusTextColor( |
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 above
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.
handleStatusTextColor is returning the color name, that is the diff.
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.
NIT: we can keep it in a variable outside jsx for better readability
| <p | ||
| className={`${styles.statusTextIndicator} ${ | ||
| styles[ | ||
| `statusTextIndicator${handleStatusTextColor( |
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 above
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.
handleStatusTextColor is returning the name, that is the diff.
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.
NIT: we can keep it in a variable outside jsx for better readability
| <span className={styles.cardStrongFont} role="button" tabIndex={0}> | ||
| {!cardDetails.endsOn ? 'TBD' : fromNowEndsOn} | ||
| </span> | ||
| <> |
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.
fragment not needed
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.
div was creating an issue here, in styling.
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 meant to say fragment is also not needed here as we have single single span element


Date: 18/02/2024
Developer Name: Palak Gupta
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Test Coverage
Screenshot 1
Additional Notes