-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
djjuhasz
commented
Feb 27, 2025
- 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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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
@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! 🎉 |
There was a problem hiding this 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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.