-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(dashboard): Allow widgets to zoom #11709
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
* Optimizes the number of queries and re-renders dashboard widgets make * Refactors intervals to be a bit more generic and allows dashboard widgets to zoom. * Adds tests
70dad63 to
56e6cf3
Compare
| }), | ||
| }; | ||
|
|
||
| export const DiscoverResult = PropTypes.arrayOf(PropTypes.shape(DiscoverResultShape)); |
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.
Should be named DiscoverResults (instead of DiscoverResult) and DiscoverResult (instead of DiscoverResultShape)
| return diffInMinutes <= TWENTY_FOUR_HOURS; | ||
| } | ||
|
|
||
| export function getInterval(datetimeObj, highFidelity) { |
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.
Maybe highFidelity=false here?
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.
Is it worth having unit tests for this function?
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.
isn't highFidelity=false implied?
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.
It's not required but it can be nice to have it always a boolean and in the method signature so reading the rest of the function won't be required to figure that out
|
|
||
| export function getInterval(datetimeObj) { | ||
| return useShortInterval(datetimeObj) ? '5m' : '30m'; | ||
| function getDiffInMinutes(datetimeObj) { |
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.
Unit tests for this as well?
| ), | ||
| timing: PropTypes.shape({ | ||
| duration_ms: PropTypes.number, | ||
| marks_ms: PropTypes.shape({ |
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.
Should we just do PropTypes.object here for now - I didn't realise this was being returned until now and it might be better to just exclude it from the api response
lynnagara
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.
👍
to zoom.
Depends on #11669