-
Notifications
You must be signed in to change notification settings - Fork 161
[APP-421] fix: enforce smallest_time_grain #8267
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
Conversation
AdityaHegde
left a comment
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.
This adds a weird state where the data displayed is not for the grain present in the url. Is there a hard requirement to show the warning? If not, would be great if we just correct it in the url as well.
web-common/src/features/dashboards/time-series/MetricsTimeSeriesCharts.svelte
Outdated
Show resolved
Hide resolved
|
This is intentional on my part, but it's not a hard requirement, so I'm happy to consider what others think about it during the applications sync. The thinking is that I want to avoid ever changing the URL without user input. We are not there yet on Explore, but I think this is a step in the right direction. I want the URL to be a reliable indicator of what the user has asked for and I think changing it only leads to hiding problems and making things difficult to debug. I want to follow the principle of what you see is what you get it. Sometimes, what you ask for is not what you're allowed to see, but at least we still follow the principle by acknowledging that it's being asked for and displaying a non-blocking error. You can imagine a situation where a user has manually changed the URL to a grain value that is too small. I think this new UX state is more transparent about what is happening and does a better job of showing that we're guarding against this kind of stuff. Also, if you use a bookmark that has an out of date value, this would make it clear that the bookmark should be updated. |
AdityaHegde
left a comment
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.
Looks good from code POV. @ericokuma @nishantmonu51 please approve if we are ok with the UX
* wip * remove logs * cleanup * test fixes * remove logs * quality check * test fix * remove logs * test fixes * tooltip adjustment * test fixes * rilltime cleanup * test updates * test fix * test fix * test fix * feedback * comment * ux feedback * use DEFAULT_TIME_RANGES map * type fix * test fix * test fix * test fix
smallest_time_graingetAggregationGrainfunctionChecklist: