-
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: Status expiry time does not show correctly when hovering the user in the chat report #41383
fix: Status expiry time does not show correctly when hovering the user in the chat report #41383
Conversation
…r in the chat report. Signed-off-by: Krishna Gupta <[email protected]>
@DylanDylann Please 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] |
src/libs/DateUtils.ts
Outdated
@@ -534,7 +534,7 @@ function getStatusUntilDate(inputDate: string): string { | |||
const endOfToday = endOfDay(now); | |||
|
|||
// If the date is equal to the end of today |
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.
@Krishna2323 Also should update this comment
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 think about this?
// endOfToday return 11:59:59.
// Use isSameSecond to ensure accuracy, otherwise, setting the time to 11:59PM would also display until tomorrow.
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.
@Krishna2323 What do you think about
// If the date is adjusted to the following day.
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.
@DylanDylann, I think we should add comment about why we are using isSameSecond instead of isSameDay. Let me know your opinion.
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.
@Krishna2323 As the pattern of this comment, this comment gives information about when this code is executed. I am ok with your suggestion to add more explanation why using isSameSecond
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.
// If the date is adjusted to the following day
// Use isSameSecond to ensure accuracy, otherwise, setting the time to 11:59PM would also display until tomorrow.
Ok, so should we add this?
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.
Ok. It looks good
// If the date is adjusted to the following day
// Use isSameSecond to ensure accuracy, otherwise, setting the time to 11:59PM would also display until tomorrow.
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.
@Krishna2323 It is a bit lengthy. I think this is enough
// If the date is adjusted to the following day
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.
@DylanDylann, updated.
@Krishna2323 Bump? |
@DylanDylann, sorry for delay, somehow I missed your suggestion. |
@Krishna2323 Also please help to merge the latest main |
Signed-off-by: Krishna Gupta <[email protected]>
…323/App into krishna2323/issue/40733
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2024-05-03.at.17.26.28.movMacOS: DesktopScreen.Recording.2024-05-03.at.17.29.12.mov |
✋ 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 production by https://github.com/marcaaron in version: 1.4.71-6 🚀
|
Details
Fixed Issues
$ #40733
PROPOSAL: #40733 (comment)
Tests
(Until tomorrow)
11:59PM
and save.(Until 11:59PM)
Offline tests
(Until tomorrow)
11:59PM
and save.(Until 11:59PM)
QA Steps
(Until tomorrow)
11:59PM
and save.(Until 11:59PM)
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4