Closed TackAdam closed 1 week ago
This fix results in a slight flicker as the scroll bar adjust after being rendered to the correct position. I have not been able to find a fix that prevents this. Have tried using useMemo as it is handled in the SpanList menu but the click event for Timeline is happening in a different area.
scrollbar resetting on rerender is not normal behavior, it might be caused by the plot rerendering. if recovering scroll position flickers, you can try to memorize the plot component instead
I see that we rely on a memo for generating the span list and inject that value directly, but I wonder why we need a memo at all? Can we use the spans as JSX and let React do the rendering? I suspect that memo is where the reset is sneaking in to begin with.
i don't think the span list is relevant for this bug, since triggering the bug doesn't need user to go to the span list view, that component was never mounted
Ah, got confused between this being the span view as a whole and there being a timeline option under the span view. But the same thing applies to the timeline, both of them are being memoized.
the gantt chart is not memorized, memorizing it could workaround this bug. you can try adding this above return
,
const ganttChart = useMemo(
() => (
<Plt
data={data.gantt}
layout={layout}
onClickHandler={onClick}
onHoverHandler={onHover}
onUnhoverHandler={onUnhover}
/>
),
[data.gantt, layout]
);
to
{toggleIdSelected === 'timeline' ? ganttChart : spanDetailTable}
Implemented Josh's suggestion and tested. All working correctly.
for completeness and good practice, the dependency array should include all dependencies. so the correct way would be using useCallback
to memorize the event handlers
const onClick = useCallback(
(event: any) => {
if (!event?.points) return;
const point = event.points[0];
if (fromApp) {
props.openSpanFlyout(point.data.spanId);
} else {
setCurrentSpan(point.data.spanId);
}
},
[props.openSpanFlyout, setCurrentSpan, fromApp]
);
const onHover = useCallback(() => {
const dragLayer = document.getElementsByClassName('nsewdrag')?.[0];
dragLayer.style.cursor = 'pointer';
}, []);
const onUnhover = useCallback(() => {
const dragLayer = document.getElementsByClassName('nsewdrag')?.[0];
dragLayer.style.cursor = '';
}, []);
and put them in the dependency array
const ganttChart = useMemo(
() => (
<Plt
data={data.gantt}
layout={layout}
onClickHandler={onClick}
onHoverHandler={onHover}
onUnhoverHandler={onUnhover}
/>
),
[data.gantt, layout, onClick, onHover, onUnhover]
);
some functions like setCurrentSpan
would never change. but if something like fromApp
changes, onClick
will be re-created with the correct behavior. And since onClick
changed and ganttChart
depends on it, ganttChart
component will also be re-created with the correct click behavior.
it likely won't make a real difference, since fromApp
shouldn't change
The backport to 2.x
failed:
The process '/usr/bin/git' failed with exit code 128
To backport manually, run these commands in your terminal:
# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-observability/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/dashboards-observability/backport-2.x
# Create a new branch
git switch --create backport/backport-1917-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cc193f6665f0f6a87846cd15016346f030a32545
# Push it to GitHub
git push --set-upstream origin backport/backport-1917-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-observability/backport-2.x
Then, create a pull request where the base
branch is 2.x
and the compare
/head
branch is backport/backport-1917-to-2.x
.
Description
This change resets the scroll bar to the position it was at prior to opening the fly-out.
Issues Resolved
https://github.com/opensearch-project/dashboards-observability/issues/1916
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.