-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
Explore: Fix showing of Prometheus data in Query inspector #28128
Conversation
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 looks good to me. Nice job on adding tests. Can you please try to convert it to use react-testing-library? I think we agreed that we want to migrate to that and write new tests with that. Also in the future it might make sense to unify this inspector component with the one we have in dashboard.
@@ -57,15 +57,15 @@ function stripPropsFromResponse(response: any) { | |||
return clonedResponse; | |||
} | |||
|
|||
interface Props { | |||
export interface Props { |
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 really just a nit (and personal and totally opinionanted, so it's even less important than a nit 😄 ), but I personally would avoid exporting anything that is not really needed outside of a module as I feel it adds noise.
In this case, Props
is not really needed outside of here apart from the test, in which case you can do:
import React, { ComponentProps } 'from 'react';
import { ExploreQueryInspector } from './ExploreQueryInspector';
type ExploreQueryInspectorProps = ComponentProps<typeof ExploreQueryInspector>;
what do you think?
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.
Uuu I like it and I agree! Will update this. Thanks!
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.
Good job! 💿
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 🚀
* Fix showing of data in explore's query inspector * Add test * Add test * Updat etest * Implement react-testing-library and remove props export * Update tests for consistency
What this PR does / why we need it:
When running Prometheus queries in Explore, the request and response wasn't shown in Inspector panel because if we have received response with
hideFromInspector === true
after, the result was overwritten. This PR fixes it.Fixed:
Before: