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: fix the mismatch between time range picker placeholder and timerange popover values #6675

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Dec 19, 2024

Summary

Related Issues / PR's

Screenshots

Before:

Screen.Recording.2024-12-16.at.6.01.29.PM.1.mov

After:

Affected Areas and Manually Tested Areas


Important

Fixes mismatch between time range picker placeholder and popover values in RangePickerModal.tsx by using useMemo and adjusting showTime and format.

  • Behavior:
    • Fixes mismatch between time range picker placeholder and popover values in RangePickerModal.tsx.
    • Uses useMemo to calculate rangeValue for RangePicker based on minTime, maxTime, and timezone.
    • Adjusts showTime to use 12-hour format and updates format to include timezone adjustment.

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

@github-actions github-actions bot added the bug Something isn't working label Dec 19, 2024
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 68cead0 in 1 minute and 16 seconds

More details
  • Looked at 56 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:60
  • Draft comment:
    The division by 1000_000 seems incorrect for converting from microseconds to milliseconds. It should be 1000 to convert from milliseconds to seconds. Please verify the time unit and adjust accordingly.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. frontend/src/components/CustomTimePicker/RangePickerModal.tsx:67
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to the className usage here.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_APaTijBaQfp3Ia64


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>

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! Incremental review on c4cc035 in 44 seconds

More details
  • Looked at 29 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/components/CustomTimePicker/RangePickerModal.tsx:60
  • Draft comment:
    The division by 1000_000 seems incorrect for converting minTime and maxTime to milliseconds. It should be 1000 instead.
			dayjs(minTime / 1000).tz(timezone.value),
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. frontend/src/components/CustomTimePicker/RangePickerModal.tsx:61
  • Draft comment:
    The division by 1000_000 seems incorrect for converting maxTime to milliseconds. It should be 1000 instead.
			dayjs(maxTime / 1000).tz(timezone.value),
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/components/CustomTimePicker/RangePickerModal.tsx:78
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This applies to the RangePickerModal.tsx file.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_nC0YjmpjNe2RCPzf


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

2 participants