-
Notifications
You must be signed in to change notification settings - Fork 217
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
ui/shared/profile/IcicleGraphArrow: Create arrow-based IcicleGraph #3326
Conversation
bee4b4d
to
49d7ba2
Compare
4da41c6
to
dbf095e
Compare
3829fde
to
9efa875
Compare
49d7ba2
to
979c4ea
Compare
2f24d6a
to
fd6853d
Compare
26e4705
to
35eb5c8
Compare
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 overall!
className="rounded-md border bg-gray-50 px-3 py-2 text-sm shadow-sm focus:border-indigo-500 focus:outline-none focus:ring-1 focus:ring-indigo-500 dark:bg-gray-900" | ||
onChange={e => setSortBy(e.target.value)} | ||
> | ||
<option value="function_name">Function Name</option> |
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.
Shall we place the select dropdown next to the Reset View
button and add a text in front. Like this:
"Reset View" "Sort by: <Select />
"
@@ -52,7 +53,8 @@ type NavigateFunction = (path: string, queryParams: any, options?: {replace?: bo | |||
|
|||
export interface FlamegraphData { | |||
loading: boolean; | |||
data?: Flamegraph; |
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.
We might need to cater for this removed prop for the grafana plugin as that's the reason for the typing error 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.
aah, I see. Let's not rename it then, but only add the table one for now. I think there is a bigger refactoring we should do anyway.
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 overall! just a few things to work over that i mentioned.
4c4f174
to
c96c7c9
Compare
There are some longer-running CI checks still running, but I would like to get this in before we have another conflict. Happy to follow up with PRs if anything is found. |
These are the frontend changes to read flame graphs from arrow records.
The backend changes are in #3069 which this PR depends on.