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

Add Date range filter #754

Merged
merged 5 commits into from
Oct 25, 2023
Merged

Add Date range filter #754

merged 5 commits into from
Oct 25, 2023

Conversation

rszwajko
Copy link
Contributor

@rszwajko rszwajko commented Oct 10, 2023

Depends on #747

Allow to filter events that belong to a time interval.
Precisely given range [A,B) a date X in the range if A <= X < B.
EDIT: changed to closed range

Key points:

  1. multiple date ranges can be chosen
  2. ISO time interval format is used to represent the range. Only date
    part is used (no time, no time zone)
  3. the range is interpreted in UTC

Reference-Url: https://en.wikipedia.org/wiki/ISO_8601#Time_intervals

Additional changes:

  1. Provide rich helper text
  2. Run date related tests additionally in UTC+02:00 zone.

Screenshot from 2023-10-19 19-28-12

image

Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

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

Before we proceed with this filter's development, it's essential to finalize the single date filter. By doing so, we can identify methods and utilities that are reusable across multiple date filters. This approach will not only ensure consistency but also make our development process more efficient. By designing the single date filter with modularity in mind, we can potentially share a significant portion of its functionality with this and other date filters. This will help us maintain a unified codebase and enhance the overall efficiency.

@rszwajko rszwajko mentioned this pull request Oct 11, 2023
@rszwajko rszwajko force-pushed the rangeFilter branch 2 times, most recently from f442d71 to ee48822 Compare October 12, 2023 20:02
@rszwajko rszwajko marked this pull request as ready for review October 12, 2023 20:05
@rszwajko rszwajko requested a review from yaacov October 12, 2023 20:05
@yaacov
Copy link
Member

yaacov commented Oct 17, 2023

Precisely given range [A,B) a date X in the range if A <= X < B.

After our discussion in the mtg, I did some google searches about the meaning of date to date, the answer I liked was
It’s ambiguous [1].

It seems the use of 'to' in date ranges can be context-dependent. For hours, like '3:00 to 5:00', it might often mean up to, but not including 5:01. However, for days, such as 'Sunday to Thursday', it usually includes the entirety of Thursday. The terms 'until' and 'through' provide clearer distinctions for exclusivity and inclusivity, respectively. Nevertheless, the key takeaway appears to be: for clarity and precision, it's always beneficial to specify explicitly.

My conclusion:
a. Since we are talking about days, the range should be from data at 00:00 to date at 23:59 witch will make the date selected look inclusive, e.g. include the last day.
b. As discussed offline, we should add a help comment that explain start day is at 00:00UTC and ends at 23:59UTC

[1] https://www.reddit.com/r/grammar/comments/7nsih7/tountil_including_date/?rdt=40133

@yaacov
Copy link
Member

yaacov commented Oct 17, 2023

After our discussion in the mtg, about the chip height,
a. is should be the same height as regular chips.
a. the hover should show the complete range string
b. the visible text can be abbreviated, maybe only the day and month without the year ?

@rszwajko
Copy link
Contributor Author

@yaacov
the suggested changes have been added as commits for better visibility.
please also check the updated screenshots.

@rszwajko rszwajko marked this pull request as ready for review October 19, 2023 18:13
@rszwajko rszwajko requested a review from yaacov October 19, 2023 18:13
Allow to filter events that belong to a time interval.
Precisely given range [A,B) a date X in the range if A <= X < B.

Key points:
1) multiple date ranges can be chosen
2) ISO time interval format is used to represent the range. Only date
   part is used (no time, no time zone)
3) the range is interpreted in UTC

Reference-Url: https://en.wikipedia.org/wiki/ISO_8601#Time_intervals
Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
Run date related tests additionally in UTC+02:00 zone.

Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
Display the entire interval string in a tooltip.

Time intervals as text are too long to be displayed in a chip.
The text is truncated which hides the end date.

Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
@yaacov
Copy link
Member

yaacov commented Oct 25, 2023

can you add a space between the bottom ant the help comment ?

comment-date

example of a gap below a help text and next item:

example-gap-bottom

Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

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

lgtm, nitpick - css issue

Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
@rszwajko
Copy link
Contributor Author

@yaacov
added padding to the bottom:
image

@sonarcloud
Copy link

sonarcloud bot commented Oct 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yaacov yaacov merged commit 9a79f63 into kubev2v:main Oct 25, 2023
5 checks passed
@yaacov yaacov mentioned this pull request Oct 25, 2023
2 tasks
rszwajko added a commit to rszwajko/tackle2-ui that referenced this pull request May 29, 2024
Create a filter widget based on DateRangeFilter developed
for Forklift plugin.
Use the new widget to filter migration waves on both start and end date.

Key points:
1. filter values are stored as strings in ISO 8601 time interval
   format (date part only) i.e. "2024-04-01/2024-05-01".
2. date range is a closed range (both ends are included)
3. browser local time zone is used
4. date format is hard-coded to "MM/DD/YYYY" to match the format
   used in the UI

Additional changes:
1. force TZ=UTC for jest unit tests to ensure the same test results
2. initialize dayjs plugins for tests

Reference-Url: kubev2v/forklift-console-plugin#754
Reference-Url: https://en.wikipedia.org/wiki/ISO_8601#Time_intervals

Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
rszwajko added a commit to rszwajko/tackle2-ui that referenced this pull request Jun 6, 2024
Create a filter widget based on DateRangeFilter developed
for Forklift plugin.
Use the new widget to filter migration waves on both start and end date.

Key points:
1. filter values are stored as strings in ISO 8601 time interval
   format (date part only) i.e. "2024-04-01/2024-05-01".
2. date range is a closed range (both ends are included)
3. browser local time zone is used
4. date format is hard-coded to "MM/DD/YYYY" to match the format
   used in the UI

Additional changes:
1. force TZ=UTC for jest unit tests to ensure the same test results
2. initialize dayjs plugins for tests

Reference-Url: kubev2v/forklift-console-plugin#754
Reference-Url: https://en.wikipedia.org/wiki/ISO_8601#Time_intervals

Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
sjd78 pushed a commit to konveyor/tackle2-ui that referenced this pull request Jun 9, 2024
Create a filter widget based on `DateRangeFilter` developed for Forklift plugin.

Use the new widget to filter migration waves on both start and end date.

Key points:
1. filter values are stored as strings in ISO 8601 time interval
   format (date part only) i.e. "2024-04-01/2024-05-01".
2. date range is a closed range (both ends are included)
3. browser local time zone is used
4. date format is hard-coded to "MM/DD/YYYY" to match the format
   used in the UI

Additional changes:
1. force TZ=UTC for jest unit tests to ensure the same test results
2. initialize dayjs plugins for tests

Reference-Url: kubev2v/forklift-console-plugin#754
Reference-Url: https://en.wikipedia.org/wiki/ISO_8601#Time_intervals

Signed-off-by: Radoslaw Szwajkowski <rszwajko@redhat.com>
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.

2 participants