From 72c8b8b2fe9973cb199eb180eec6e356ac03f8bc Mon Sep 17 00:00:00 2001 From: oltionchampari Date: Wed, 7 Feb 2024 09:01:37 +0100 Subject: [PATCH 1/6] fix: merge visConfig defaults before painting --- src/vis/EagerVis.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vis/EagerVis.tsx b/src/vis/EagerVis.tsx index a02b0d955..48e22dd78 100644 --- a/src/vis/EagerVis.tsx +++ b/src/vis/EagerVis.tsx @@ -258,7 +258,7 @@ export function EagerVis({ }); }, []); - React.useEffect(() => { + React.useLayoutEffect(() => { const vis = getVisByType(inconsistentVisConfig?.type); if (vis) { const newConfig = vis.mergeConfig(columns, inconsistentVisConfig); From 185707334589bc92260a9c2799071b434f8014f2 Mon Sep 17 00:00:00 2001 From: oltionchampari Date: Wed, 7 Feb 2024 10:36:44 +0100 Subject: [PATCH 2/6] fix: null category in heatmap --- src/vis/heatmap/HeatmapText.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vis/heatmap/HeatmapText.tsx b/src/vis/heatmap/HeatmapText.tsx index 37744f1e7..28b0fbd39 100644 --- a/src/vis/heatmap/HeatmapText.tsx +++ b/src/vis/heatmap/HeatmapText.tsx @@ -26,7 +26,7 @@ export function HeatmapText({ isImmediate: boolean; }) { const labelSpacing = useMemo(() => { - const maxLabelLength = d3.max(yScale.domain().map((m) => m.length)); + const maxLabelLength = d3.max(yScale.domain().map((m) => m?.length)); return maxLabelLength > 5 ? 35 : maxLabelLength * 7; }, [yScale]); From 92ec4fb928d1012fd6f78b6e5e778d16e2a59c4f Mon Sep 17 00:00:00 2001 From: oltionchampari Date: Thu, 8 Feb 2024 11:58:35 +0100 Subject: [PATCH 3/6] fix: heatmap invalid settings --- src/vis/heatmap/HeatmapGrid.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/vis/heatmap/HeatmapGrid.tsx b/src/vis/heatmap/HeatmapGrid.tsx index 05af2e414..5d173273a 100644 --- a/src/vis/heatmap/HeatmapGrid.tsx +++ b/src/vis/heatmap/HeatmapGrid.tsx @@ -34,11 +34,8 @@ export function HeatmapGrid({ return ( - {status === 'pending' ? ( - - ) : !hasAtLeast2CatCols ? ( - - ) : ( + {status === 'pending' && } + {status === 'success' && hasAtLeast2CatCols && ( )} + {status === 'success' && !hasAtLeast2CatCols && ( + + )} ); } From 1d9b01d6c499ade30b3ea402e7d72641b5136e45 Mon Sep 17 00:00:00 2001 From: oltionchampari Date: Thu, 8 Feb 2024 13:29:08 +0100 Subject: [PATCH 4/6] fix: try a simpler solution --- src/vis/EagerVis.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/vis/EagerVis.tsx b/src/vis/EagerVis.tsx index 48e22dd78..97a3c44a4 100644 --- a/src/vis/EagerVis.tsx +++ b/src/vis/EagerVis.tsx @@ -245,7 +245,9 @@ export function EagerVis({ const setExternalConfigRef = useSyncedRef(setExternalConfig); useEffect(() => { - setExternalConfigRef.current?.(visConfig); + if (visConfig) { + setExternalConfigRef.current?.(visConfig); + } // eslint-disable-next-line react-hooks/exhaustive-deps }, [JSON.stringify(visConfig), setExternalConfigRef]); @@ -258,7 +260,7 @@ export function EagerVis({ }); }, []); - React.useLayoutEffect(() => { + React.useEffect(() => { const vis = getVisByType(inconsistentVisConfig?.type); if (vis) { const newConfig = vis.mergeConfig(columns, inconsistentVisConfig); From 426854c35fbd6b577a978dc17bb909c1e4f68ece Mon Sep 17 00:00:00 2001 From: oltionchampari Date: Thu, 8 Feb 2024 14:06:46 +0100 Subject: [PATCH 5/6] fix: use style instead of sx --- src/vis/heatmap/HeatmapGrid.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vis/heatmap/HeatmapGrid.tsx b/src/vis/heatmap/HeatmapGrid.tsx index 5d173273a..2a5eed012 100644 --- a/src/vis/heatmap/HeatmapGrid.tsx +++ b/src/vis/heatmap/HeatmapGrid.tsx @@ -33,7 +33,7 @@ export function HeatmapGrid({ }, []); return ( - + {status === 'pending' && } {status === 'success' && hasAtLeast2CatCols && ( Date: Fri, 9 Feb 2024 09:38:47 +0100 Subject: [PATCH 6/6] refactor: use useUncontrolled hook in EagerVis --- src/vis/EagerVis.tsx | 162 ++++++++++++++++--------------------------- 1 file changed, 61 insertions(+), 101 deletions(-) diff --git a/src/vis/EagerVis.tsx b/src/vis/EagerVis.tsx index a003797ae..5c0894970 100644 --- a/src/vis/EagerVis.tsx +++ b/src/vis/EagerVis.tsx @@ -204,78 +204,40 @@ export function EagerVis({ const { getVisByType } = useVisProvider(); - // Each time you switch between vis config types, there is one render where the config is inconsistent with the type before the merge functions in the useEffect below can be called. - // To ensure that we never render an incosistent config, keep a consistent and a current in the config. Always render the consistent. // eslint-disable-next-line @typescript-eslint/naming-convention - const [{ consistent: visConfig, current: inconsistentVisConfig }, _setVisConfig] = React.useState<{ - consistent: BaseVisConfig; - current: BaseVisConfig; - }>( - externalConfig - ? { consistent: null, current: externalConfig } + const [_visConfig, _setVisConfig] = useUncontrolled({ + value: externalConfig?.type ? getVisByType(externalConfig?.type)?.mergeConfig(columns, externalConfig) : null, + defaultValue: externalConfig + ? null : columns.filter((c) => c.type === EColumnTypes.NUMERICAL).length > 1 - ? { - consistent: null, - current: { - type: ESupportedPlotlyVis.SCATTER, - numColumnsSelected: [], - color: null, - numColorScaleType: ENumericalColorScaleType.SEQUENTIAL, - shape: null, - dragMode: EScatterSelectSettings.RECTANGLE, - alphaSliderVal: 0.5, - } as BaseVisConfig, - } - : { - consistent: null, - current: { - type: ESupportedPlotlyVis.BAR, - multiples: null, - group: null, - direction: EBarDirection.HORIZONTAL, - display: EBarDisplayType.ABSOLUTE, - groupType: EBarGroupingType.STACK, - numColumnsSelected: [], - catColumnSelected: null, - aggregateColumn: null, - aggregateType: EAggregateTypes.COUNT, - } as BaseVisConfig, - }, - ); - - const setExternalConfigRef = useSyncedRef(setExternalConfig); - useEffect(() => { - if (visConfig) { - setExternalConfigRef.current?.(visConfig); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [JSON.stringify(visConfig), setExternalConfigRef]); - - const setVisConfig = React.useCallback((newConfig: BaseVisConfig) => { - _setVisConfig((oldConfig) => { - return { - current: newConfig, - consistent: oldConfig.current.type !== newConfig.type ? oldConfig.consistent : newConfig, - }; - }); - }, []); - - React.useEffect(() => { - const vis = getVisByType(inconsistentVisConfig?.type); - if (vis) { - const newConfig = vis.mergeConfig(columns, inconsistentVisConfig); - _setVisConfig({ current: newConfig, consistent: newConfig }); - } - - // DANGER:: this useEffect should only occur when the visConfig.type changes. adding visconfig into the dep array will cause an infinite loop. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [inconsistentVisConfig?.type, getVisByType]); + ? ({ + type: ESupportedPlotlyVis.SCATTER, + numColumnsSelected: [], + color: null, + numColorScaleType: ENumericalColorScaleType.SEQUENTIAL, + shape: null, + dragMode: EScatterSelectSettings.RECTANGLE, + alphaSliderVal: 0.5, + } as BaseVisConfig) + : ({ + type: ESupportedPlotlyVis.BAR, + multiples: null, + group: null, + direction: EBarDirection.HORIZONTAL, + display: EBarDisplayType.ABSOLUTE, + groupType: EBarGroupingType.STACK, + numColumnsSelected: [], + catColumnSelected: null, + aggregateColumn: null, + aggregateType: EAggregateTypes.COUNT, + } as BaseVisConfig), + onChange: setExternalConfig, + }); - useEffect(() => { - if (externalConfig) { - setVisConfig(externalConfig); - } - }, [externalConfig, setVisConfig]); + const setVisConfig = (v: BaseVisConfig) => { + const withDefaults = getVisByType(v.type)?.mergeConfig(columns, v); + _setVisConfig(withDefaults); + }; // Converting the selected list into a map, since searching through the list to find an item is common in the vis components. const selectedMap: { [key: string]: boolean } = useMemo(() => { @@ -311,17 +273,17 @@ export function EagerVis({ }; }, [colors]); - if (!visConfig) { - return
; - } - const commonProps = { showSidebar, setShowSidebar, enableSidebar, }; - const Renderer = getVisByType(visConfig?.type)?.renderer; + const Renderer = getVisByType(_visConfig?.type)?.renderer; + + if (!_visConfig || !Renderer) { + return null; + } return ( setShowSidebar(!showSidebar)} /> : null} - {Renderer ? ( - - ) : null} + {showSidebar ? ( - setShowSidebar(false)}> - + setShowSidebar(false)}> + ) : null}