Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Jan 22, 2025

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 in RangePickerModal.tsx.

  • Behavior:
    • Sets defaultValue for showTime in RangePicker in RangePickerModal.tsx to prevent disabling today's date.
    • Uses dayjs().tz(timezone.value) for both start and end default times.

This description was created by Ellipsis for 8c7ae68. It will automatically update as commits are pushed.

@ahmadshaheer ahmadshaheer requested a review from YounixM as a code owner January 22, 2025 04:53
@github-actions github-actions bot added the bug Something isn't working label Jan 22, 2025
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant