fix(TenantDashboard): hide if charts not enabled #675
Conversation
artemmufazalov
force-pushed
the
hide-disabled-charts
branch
from
6cf5c4c to
b91648b
Compare
artemmufazalov
force-pushed
the
hide-disabled-charts
branch
from
b91648b to
ac02a60
Compare
| const [chartDataStatus, setChartDataStatus] = useState<ChartDataStatus>('loading'); | ||
|
|
||
| // Update status in useEffect instead of fetchChartData, so data won't be fetched on callback update | ||
| useEffect(() => { |
There was a problem hiding this comment.
I think we should resolve problem with changable callback. It seems that such a combination of useState and useEffect may cause a lot of unneeded re-renders. Let's try to fix it.
There was a problem hiding this comment.
Replaced it with useEffect, dependant on chart state. It should not cause additional rerenders, since component already rerenders on reducer state change
| charts={cpuDashboardConfig} | ||
| onDashboardLoad={() => setDashboardLoading(false)} | ||
| /> | ||
| <TopNodesByLoad path={path} additionalNodesProps={additionalNodesProps} /> |
There was a problem hiding this comment.
Why are all this components in one LoadingContainer and their visibility depends on dashboardLoading ?
I would better move loading logic to TenantDashboard. If you do so, you won't need LoadingContainer at all.
There was a problem hiding this comment.
uplot charts don't render correctly in components with display:none. Fixed this issue with a hack, removed additional containers
| // Use visibility:hidden to preserve loading content size | ||
| // Also it helps to properly render charts and virtualized elements | ||
| // that didn't render properly with display:none since they need parent container sizes | ||
| export const LoadingContainer = ({ |
There was a problem hiding this comment.
I'm not sure that this additional wrapper above Loader is really essential. I would better stick to basic library component without wrappers.
|
|
||
| export const TenantDashboard = ({charts, onDashboardLoad}: TenantDashboardProps) => { | ||
| const [dashboardStatus, setDashboardStatus] = useState<TenantDashboardStatus>(); | ||
| const chartsStatuses = useRef<DashboardsChartsStatuses>({}); |
There was a problem hiding this comment.
It seem a bit redundant: you need to know if at least one chart is succeed. So, no need to check charts id's:
const [showDashboard, setShowDashboard] = useState(false);
... one chart was loaded => setShowDashboard(true) ...
... no chats was loaded => do nothing ...
if (!showDashboard) {
return null;
}
There was a problem hiding this comment.
Yep, updated it. Didn't figured out in the beginning, that we can prevent charts refetches with autorefresh variable
artemmufazalov
force-pushed
the
hide-disabled-charts
branch
from
ac02a60 to
4064b32
Compare
| * @link https://github.com/ydb-platform/ydb-embedded-ui/issues/659 | ||
| * @todo disable only for specific errors ('GraphShard is not enabled') after ydb-stable-24 is generally used | ||
| */ | ||
| const handleChartDataStatusChange = (chartStatus: ChartDataStatus) => { |
There was a problem hiding this comment.
It seems that you are interested only in success finish. So, lets rename this to let's say handleChartLoadingSucceeded and execute it only in one case - if chart was successfully loaded.
There was a problem hiding this comment.
I made quite a universal solution, to make it possible to extend it later - hide charts only on specific errors.
Currently they are hidden on every error, including timeouts when node is not healthy, some internal problems, etc. The reason - versions without render endpoint don't return specific error.
Further charts should be hidden only on specific errors, when GraphShard is not enabled - added todo not to forger.
| observer.current?.unobserve(el); | ||
| } | ||
| }; | ||
| }, [ref]); |
There was a problem hiding this comment.
Ref can't be a dependency.
| // uplot has the issue, that it doesn't render correctly inside not visible elements | ||
| // so if chart is used inside component with display:none, it will be empty, when visibility change | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
| const isChartVisible = useIsElementVisible(containerRef); |
There was a problem hiding this comment.
It looks much more straightforward to pass isChartVisible property from parent.
There was a problem hiding this comment.
It's more straightforward, but it makes you to pass visible prop whenever you want to hide some component, that has charts inside. For example, if I'd want to add NotRenderUntilFirstVisible logic to tenant diagnostics tabs, it won't be possible without props drilling to ensure charts properly hidden and shown
There was a problem hiding this comment.
Yes, but it seems to be fair. If you want to hide component - you should do it explicitly. Logic with ResizeObserver is more complicated and hidden.
artemmufazalov
force-pushed
the
hide-disabled-charts
branch
from
00359ce to
ead5901
Compare
Issue: #659
Charts should be hidden, if they are not enabled. Since charts dashboard is quite big and causes layout shifts, it should be hidden under loader together with other content until charts data loads and it's clear, whether to show dashboard or not
Node, where charts are not enabled: https://nda.ya.ru/t/OnrxodIe74UwCV
Node, where charts enabled: https://nda.ya.ru/t/7w84LYSB74UwDE