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

Audit Log Date Range Picker update #451

Conversation

robertandremitchell
Copy link
Collaborator

@robertandremitchell robertandremitchell commented Mar 21, 2025

PULL REQUEST

Summary

This initializes a DateRangePicker component in line with the frontend guidance:
image
Screenshot 2025-03-26 at 10 29 14 AM
Screenshot 2025-03-26 at 10 29 30 AM
Screenshot 2025-03-26 at 10 30 58 AM

It also adds tests of date range to AuditLog to ensure functionality.

This moves the DateRangePicker logic out of AuditLog and establishes what could be the prototype for timeboxing, since that will need a date range picker in addition to the other components: https://www.figma.com/design/Iuw9me6kYftBF4WTCEsCZz/Query-Connector?node-id=1757-1955&p=f&m=dev.

However, per discussions with @mikang we should avoid full implementation since this more simplified date range is all that is needed for MVP.

Related Issue

Fixes #441

Additional Information

Anything else the review team should know?

The only edit I made to the design was the inclusion of a Clear button. I found that when playing around with dates that there was not a clear way to reset the dates without refreshing. This is because I have the textbox set to readOnly so that users can only implement date ranges via the picker and not trying to type in two dates.

The other thing is that I have currently commented out the onClick logic that opens the calendars on click. I had trouble getting the tests to "ignore" that logic to type dates. Further, having the calendar open automatically does seem to go against accessibility guidance, which states:

Always allow a user to type in the date manually. Usability testing suggests some people prefer manually typing the date rather than using the calendar picker. Whenever possible, keep the keyboard active so people can enter in the date information without having to use the picker.

So, I'm curious if we want to add that back in. If so, I will need to figure out the best way to get tests to work with that logic (the rabbit hole went deep...).

Checklist

  • Descriptive Pull Request title
  • Link to relevant issues
  • Provide necessary context for design reviewers
  • Ensure test coverage is above agreed upon threshold
  • Update documentation

Copy link

github-actions bot commented Mar 21, 2025

Coverage report for ./query-connector

St.
Category Percentage Covered / Total
🟡 Statements
60.7% (+1.29% 🔼)
1798/2962
🔴 Branches
51.87% (+2.62% 🔼)
540/1041
🟡 Functions
64.03% (+1.18% 🔼)
372/581
🟡 Lines
60.62% (+1.28% 🔼)
1695/2796
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / DateRangePicker.tsx
98.82% 92.31% 100% 98.73%

Test suite run success

162 tests passing in 20 suites.

Report generated by 🧪jest coverage report action from 9d85ffc

@robertandremitchell robertandremitchell marked this pull request as ready for review March 26, 2025 14:25
Copy link
Collaborator

@fzhao99 fzhao99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work on this Rob! left a couple nits, but nothing blocking

@mikang
Copy link
Collaborator

mikang commented Mar 27, 2025

This looks great!!! Thanks, @robertandremitchell !

Copy link
Collaborator

@mikang mikang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!!

Copy link
Collaborator

@katyasoup katyasoup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! There are a couple validation things I noticed that shouldn't be blockers, but maybe we'd want to make a new ticket for/ follow-up on:

  • seems to trigger early, ex. enter a start date, then any text in end date triggers warnings until the full date is typed out. Maybe an "Apply" button or similar would solve?
Screenshot 2025-03-28 at 11 19 05 Screenshot 2025-03-28 at 11 19 13
  • Invalid format warning shows but filtering works as expected underneath: (enter 3/1/24 for start in this ex - year is corrected to YYYY format but month/day stays single digit and proper filtering is applied to results):
Screenshot 2025-03-28 at 11 25 38

@robertandremitchell
Copy link
Collaborator Author

Looks great! There are a couple validation things I noticed that shouldn't be blockers, but maybe we'd want to make a new ticket for/ follow-up on:

  • seems to trigger early, ex. enter a start date, then any text in end date triggers warnings until the full date is typed out. Maybe an "Apply" button or similar would solve?

Screenshot 2025-03-28 at 11 19 05 Screenshot 2025-03-28 at 11 19 13

  • Invalid format warning shows but filtering works as expected underneath: (enter 3/1/24 for start in this ex - year is corrected to YYYY format but month/day stays single digit and proper filtering is applied to results):
Screenshot 2025-03-28 at 11 25 38

Looks great! There are a couple validation things I noticed that shouldn't be blockers, but maybe we'd want to make a new ticket for/ follow-up on:

  • seems to trigger early, ex. enter a start date, then any text in end date triggers warnings until the full date is typed out. Maybe an "Apply" button or similar would solve?

Screenshot 2025-03-28 at 11 19 05 Screenshot 2025-03-28 at 11 19 13

  • Invalid format warning shows but filtering works as expected underneath: (enter 3/1/24 for start in this ex - year is corrected to YYYY format but month/day stays single digit and proper filtering is applied to results):
Screenshot 2025-03-28 at 11 25 38

Looks great! There are a couple validation things I noticed that shouldn't be blockers, but maybe we'd want to make a new ticket for/ follow-up on:

  • seems to trigger early, ex. enter a start date, then any text in end date triggers warnings until the full date is typed out. Maybe an "Apply" button or similar would solve?

Screenshot 2025-03-28 at 11 19 05 Screenshot 2025-03-28 at 11 19 13

  • Invalid format warning shows but filtering works as expected underneath: (enter 3/1/24 for start in this ex - year is corrected to YYYY format but month/day stays single digit and proper filtering is applied to results):
Screenshot 2025-03-28 at 11 25 38

No dice fixing locally yesterday, unfortunately :/ . I'm sure there is a way to handle date error handling more gracefully, but the main thing I ran into was the balance between having a permissive date values (YYYY.MM.DD, MM/DD/YYYY, MM-DD-YY, etc.) but still triggering the errors when they became valid. We could try to enforce MM/DD/YYYY as the only datetype we support, but it does feel a little restrictive, even if our error messaging then recommends they use that format.

Made a follow-up ticket to investigate for the sake of merging this since date range picker works: https://linear.app/skylight-cdc/issue/QUE-223/update-date-range-error-handling.

@robertandremitchell robertandremitchell merged commit 7d4ec74 into main Apr 1, 2025
12 checks passed
@robertandremitchell robertandremitchell deleted the rob/441-frontend-implement-new-daterangepicker-design-for-audit-log branch April 1, 2025 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend: Implement new DateRangePicker design for Audit Log
4 participants