-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: prevent today's date disabling in range picker by setting default time values #6876
base: main
Are you sure you want to change the base?
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Reviewed everything up to 8c7ae68 in 36 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/components/CustomTimePicker/RangePickerModal.tsx:74
- Draft comment:
Setting both default start and end times to the current time may not be ideal for a range picker. Consider setting a sensible default range, such as start of the day to end of the day, or a specific duration. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The defaultValue only affects the initial time suggestion when opening the picker. The actual range values are properly handled through the rangeValue prop. The current implementation using the current time as default for both start/end times is reasonable since it gives users a starting point they can easily adjust. A different default range wouldn't necessarily be more useful.
I could be wrong about the UX implications - maybe having different default times would make it easier for users to select common time ranges.
The current implementation is flexible and neutral - users can easily adjust the times as needed, and the actual range values are properly handled elsewhere. Making assumptions about what range users want could be more confusing.
The comment should be deleted as it suggests a change that isn't clearly beneficial and the current implementation is reasonable.
2. frontend/src/components/CustomTimePicker/RangePickerModal.tsx:71
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_e2DoFD41UQIHbgG6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Summary
This PR fixes the issue of today's date being disabled intermittently in the custom time range
Related Issues / PR's
Screenshots
Before:
2025-01-22.09-11-17.mov
After:
2025-01-22.09-18-39.mov
Affected Areas and Manually Tested Areas
Custom time range selector
Important
Sets default time values in
RangePicker
to prevent disabling today's date inRangePickerModal.tsx
.defaultValue
forshowTime
inRangePicker
inRangePickerModal.tsx
to prevent disabling today's date.dayjs().tz(timezone.value)
for both start and end default times.This description was created by for 8c7ae68. It will automatically update as commits are pushed.