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

Pass start and end times to TimeDropdown #1139

Merged
merged 3 commits into from
Mar 3, 2025

Conversation

djjuhasz
Copy link
Collaborator

  • Allow passing an initial start and end time to the TimeDropown component
  • Pass the "earliest created date" and "latest created date" parameters from the current route to the "Created" date/time filter.

- Allow passing an initial start and end time to the TimeDropown
  component
- Pass the "earliest created date" and "latest created date" parameters
  from the current route to the "Created" date/time filter.
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.71%. Comparing base (5e255ea) to head (71ce8d1).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1139      +/-   ##
==========================================
- Coverage   54.75%   54.71%   -0.04%     
==========================================
  Files         105      105              
  Lines        7810     7810              
==========================================
- Hits         4276     4273       -3     
- Misses       3265     3267       +2     
- Partials      269      270       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Also upgrade @vitest/coverage-v8 from 2.1.9 to 3.0.7.
- Update reset button unit test so it sets a time before triggering the
  reset
- Change "clear" to "reset" for the dropdown reset functionality
- Call `vi.setSystemTime()` before mounting the component, which solves
  many weird testing behaviours and fixes the test coverage reporting
@djjuhasz djjuhasz requested a review from jraddaoui February 28, 2025 02:01
@djjuhasz
Copy link
Collaborator Author

@jraddaoui this PR contains three distinct commits but they are all small. Most importantly for my sanity I finally figured out why the test coverage report was not working properly! 🎉

Copy link
Collaborator

@jraddaoui jraddaoui left a comment

Choose a reason for hiding this comment

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

LGTM, just some thoughts for the future, thanks @djjuhasz!

@@ -41,6 +43,14 @@ const endTime = ref<Date | null>(null);

onMounted(() => {
if (el.value) dropdown.value = new Dropdown(el.value);
if (props.start) {
startTime.value = props.start;
btnLabel.value = defaultLabel + ": Custom";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big deal, but this could be misleading if the date comes from a preset value. The times will change for those preset values so it kind of makes sense to make it custom if you want to keep the exact time that was used at the time of filtering, although the end date will vary as it's never set in those presets.

I think it would make more sense to have the preset ranges as part of the dashboard URL createdAtRange=3h and then transform them before they are sent to the API to be able to keep showing the last 3 hour SIPs.

Just a note, maybe for the future if we think it deserves the effort, not saying we should change it now as it will require modifying the actual implementation quite a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good points @jraddaoui. I don't know if Enduro users will want to have URLs that represent a constant time span (in which case we'd need to set the end time as you've mentioned) or would prefer for the URL to always represent the 3 hours before the current time. 🤔

As you say, I think this is something we can address in the future based on user feedback.

@djjuhasz djjuhasz merged commit 67d48cb into main Mar 3, 2025
14 checks passed
@djjuhasz djjuhasz deleted the dev/issue-1102-date-filter-fix branch March 3, 2025 20:05
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