fix(perf): Perf Landing v3 UI Tweaks #29812
Conversation
This disables the zoom in the histogram widget for now until we introduce a reset or some way to zoom out.
Frontend other can have both http requests for retrieving data as well as resource ops due to chunk loading techniques (webpack)
…the data is null for the selectedListIndex picking out of the array.
size-limit report
|
…ly to related errors and related issues.
k-fish
force-pushed
the
fix/perf-landing-ui-tweaks
branch
from
d8f9ca0 to
02fc67a
Compare
Zylphrex
left a comment
Zylphrex
left a comment
There was a problem hiding this comment.
Overall looks fine, though I'm not sure the hook dependencies are correct or not.
| field={props.fields[0]} | ||
| chartData={provided.widgetData.chart?.data?.[props.fields[0]]} | ||
| disableXAxis | ||
| disableZoom |
There was a problem hiding this comment.
We've never had an un-zoom feature, is there a reason why it's a necessity now?
There was a problem hiding this comment.
You can zoom in a bunch of the widgets but there is no way to reset and unzoom, we're also currently not making 2nd requests to get the zoomed in data, I'd rather just exclude the ability for now.
| }, | ||
| }; | ||
| }, [props.eventView.query, props.fields[0], props.organization.slug]); | ||
| const chart = useMemo<QueryDefinition<DataType, WidgetDataResult>>( |
There was a problem hiding this comment.
I'm not sure I understand why the useMemo here is needed. If you wrap the memorized result inside of an object like you have below, it will always trigger a re-render of the child component since the reference to Queries will be different even if chart has been memorized right?
There was a problem hiding this comment.
Re-rendering the same component is fine now, building a new component inside an object each render isn't. Since it wasn't part of the jsx in the return iirc it was being rebuilt each time.
| transform: transformEventsRequestToArea, | ||
| }; | ||
| }, [props.eventView.query, field, props.organization.slug, selectedListIndex]); | ||
| }, [props.eventView, field, props.organization.slug, selectedListIndex]); |
There was a problem hiding this comment.
Can't say with confidence that I have a strong grasp on what this will do but won't this trigger a re-render on every location change because eventView is re-derived each time, resulting in a new reference?
There was a problem hiding this comment.
Yeah it schedules a rerender, but eventView is no longer being passed to the query component, so the component itself never refires. Re-renders are fine, but re-firing the api request is not. The eventView has more than just the query to look for (in this case statsPeriod was changing), I'd rather not figure out primitives for every facet of eventView that could change and just let the existing component refetch mechanism deal with it.
Summary
This include quite a few small fixes to the functionality of the landing page widgets, split up by commit