Flamegraph: Refactor css styling (#78698)

.betterer.results

@@ -653,9 +653,7 @@ exports[`better eslint`] = {
     ],
     "packages/grafana-flamegraph/src/TopTable/FlameGraphTopTableContainer.tsx:5381": [
       [0, 0, 0, "Styles should be written using objects.", "0"],
-      [0, 0, 0, "Styles should be written using objects.", "1"],
+      [0, 0, 0, "Styles should be written using objects.", "1"]
-      [0, 0, 0, "Styles should be written using objects.", "2"],
-      [0, 0, 0, "Styles should be written using objects.", "3"]
     ],
     "packages/grafana-runtime/src/analytics/types.ts:5381": [
       [0, 0, 0, "Unexpected any. Specify a different type.", "0"]

packages/grafana-flamegraph/src/FlameGraph/FlameGraph.tsx

@@ -178,7 +178,6 @@ const getStyles = () => ({
   graph: css`
     label: graph;
     overflow: auto;
-    height: 100%;
     flex-grow: 1;
     flex-basis: 50%;
   `,

packages/grafana-flamegraph/src/FlameGraph/FlameGraphCanvas.tsx

@@ -245,7 +245,6 @@ const getStyles = () => ({
   graph: css({
     label: 'graph',
     overflow: 'auto',
-    height: '100%',
     flexGrow: 1,
     flexBasis: '50%',
   }),

packages/grafana-flamegraph/src/FlameGraphContainer.tsx

@@ -98,7 +98,7 @@ const FlameGraphContainer = ({
     return new FlameGraphDataContainer(data, { collapsing: !disableCollapsing }, theme);
   }, [data, theme, disableCollapsing]);
   const [colorScheme, setColorScheme] = useColorScheme(dataContainer);
-  const styles = getStyles(theme, vertical);
+  const styles = getStyles(theme);
 
   // If user resizes window with both as the selected view
   useEffect(() => {
@@ -144,6 +144,65 @@ const FlameGraphContainer = ({
     return null;
   }
 
+  const flameGraph = (
+    <FlameGraph
+      data={dataContainer}
+      rangeMin={rangeMin}
+      rangeMax={rangeMax}
+      search={search}
+      setRangeMin={setRangeMin}
+      setRangeMax={setRangeMax}
+      onItemFocused={(data) => setFocusedItemData(data)}
+      focusedItemData={focusedItemData}
+      textAlign={textAlign}
+      sandwichItem={sandwichItem}
+      onSandwich={(label: string) => {
+        resetFocus();
+        setSandwichItem(label);
+      }}
+      onFocusPillClick={resetFocus}
+      onSandwichPillClick={resetSandwich}
+      colorScheme={colorScheme}
+      showFlameGraphOnly={showFlameGraphOnly}
+      collapsing={!disableCollapsing}
+    />
+  );
+
+  const table = (
+    <FlameGraphTopTableContainer
+      data={dataContainer}
+      onSymbolClick={onSymbolClick}
+      search={search}
+      sandwichItem={sandwichItem}
+      onSandwich={setSandwichItem}
+      onSearch={setSearch}
+      onTableSort={onTableSort}
+    />
+  );
+
+  let body;
+  if (showFlameGraphOnly || selectedView === SelectedView.FlameGraph) {
+    body = flameGraph;
+  } else if (selectedView === SelectedView.TopTable) {
+    body = <div className={styles.tableContainer}>{table}</div>;
+  } else if (selectedView === SelectedView.Both) {
+    if (vertical) {
+      body = (
+        <div>
+          <div className={styles.verticalGraphContainer}>{flameGraph}</div>
+          <div className={styles.verticalTableContainer}>{table}</div>
+        </div>
+      );
+    } else {
+      body = (
+        <div className={styles.horizontalContainer}>
+          <div className={styles.horizontalTableContainer}>{table}</div>
+          <div className={styles.horizontalGraphContainer}>{flameGraph}</div>
+        </div>
+      );
+    }
+  }
+
   return (
     // We add the theme context to bridge the gap if this is rendered in non grafana environment where the context
     // isn't already provided.
@@ -178,45 +237,7 @@ const FlameGraphContainer = ({
           />
         )}
 
-        <div className={styles.body}>
+        <div className={styles.body}>{body}</div>
-          {!showFlameGraphOnly && selectedView !== SelectedView.FlameGraph && (
-            <FlameGraphTopTableContainer
-              data={dataContainer}
-              onSymbolClick={onSymbolClick}
-              height={selectedView === SelectedView.TopTable || vertical ? 600 : undefined}
-              search={search}
-              sandwichItem={sandwichItem}
-              onSandwich={setSandwichItem}
-              onSearch={setSearch}
-              onTableSort={onTableSort}
-              vertical={vertical}
-            />
-          )}
-
-          {selectedView !== SelectedView.TopTable && (
-            <FlameGraph
-              data={dataContainer}
-              rangeMin={rangeMin}
-              rangeMax={rangeMax}
-              search={search}
-              setRangeMin={setRangeMin}
-              setRangeMax={setRangeMax}
-              onItemFocused={(data) => setFocusedItemData(data)}
-              focusedItemData={focusedItemData}
-              textAlign={textAlign}
-              sandwichItem={sandwichItem}
-              onSandwich={(label: string) => {
-                resetFocus();
-                setSandwichItem(label);
-              }}
-              onFocusPillClick={resetFocus}
-              onSandwichPillClick={resetSandwich}
-              colorScheme={colorScheme}
-              showFlameGraphOnly={showFlameGraphOnly}
-              collapsing={!disableCollapsing}
-            />
-          )}
-        </div>
       </div>
     </ThemeContext.Provider>
   );
@@ -234,12 +255,13 @@ function useColorScheme(dataContainer: FlameGraphDataContainer | undefined) {
   return [colorScheme, setColorScheme] as const;
 }
 
-function getStyles(theme: GrafanaTheme2, vertical?: boolean) {
+function getStyles(theme: GrafanaTheme2) {
   return {
     container: css({
       label: 'container',
+      overflow: 'auto',
       height: '100%',
-      display: vertical ? 'block' : 'flex',
+      display: 'flex',
       flex: '1 1 0',
       flexDirection: 'column',
       minHeight: 0,
@@ -247,12 +269,39 @@ function getStyles(theme: GrafanaTheme2, vertical?: boolean) {
     }),
     body: css({
       label: 'body',
-      display: 'flex',
       flexGrow: 1,
+    }),
+
+    tableContainer: css({
+      // This is not ideal for dashboard panel where it creates a double scroll. In a panel it should be 100% but then
+      // in explore we need a specific height.
+      height: 800,
+    }),
+
+    horizontalContainer: css({
+      label: 'horizontalContainer',
+      display: 'flex',
       minHeight: 0,
-      height: vertical ? undefined : '100vh',
+      flexDirection: 'row',
-      flexDirection: vertical ? 'column-reverse' : 'row',
       columnGap: theme.spacing(1),
+      width: '100%',
+    }),
+
+    horizontalGraphContainer: css({
+      flexBasis: '50%',
+    }),
+
+    horizontalTableContainer: css({
+      flexBasis: '50%',
+      maxHeight: 800,
+    }),
+
+    verticalGraphContainer: css({
+      marginBottom: theme.spacing(1),
+    }),
+
+    verticalTableContainer: css({
+      height: 800,
     }),
   };
 }

packages/grafana-flamegraph/src/TopTable/FlameGraphTopTableContainer.tsx

@@ -29,17 +29,15 @@ import { TableData } from '../types';
 type Props = {
   data: FlameGraphDataContainer;
   onSymbolClick: (symbol: string) => void;
-  height?: number;
   search?: string;
   sandwichItem?: string;
   onSearch: (str: string) => void;
   onSandwich: (str?: string) => void;
   onTableSort?: (sort: string) => void;
-  vertical?: boolean;
 };
 
 const FlameGraphTopTableContainer = React.memo(
-  ({ data, onSymbolClick, height, search, onSearch, sandwichItem, onSandwich, onTableSort, vertical }: Props) => {
+  ({ data, onSymbolClick, search, onSearch, sandwichItem, onSandwich, onTableSort }: Props) => {
     const table = useMemo(() => {
       // Group the data by label, we show only one row per label and sum the values
       // TODO: should be by filename + funcName + linenumber?
@@ -57,21 +55,14 @@ const FlameGraphTopTableContainer = React.memo(
       return table;
     }, [data]);
 
-    const rowHeight = 35;
+    const styles = useStyles2(getStyles);
-    // When we use normal layout we size the table to have the same height as the flamegraph to look good side by side.
-    // In vertical layout we don't need that so this is a bit arbitrary. We want some max limit
-    // so we don't show potentially thousands of rows at once which can hinder performance (the table is virtualized
-    // so with some max height it handles it fine)
-    const tableHeight = vertical ? Math.min(Object.keys(table).length * rowHeight, 800) : 0;
-
-    const styles = useStyles2(getStyles, tableHeight);
     const theme = useTheme2();
 
     const [sort, setSort] = useState<TableSortByFieldState[]>([{ displayName: 'Self', desc: true }]);
 
     return (
       <div className={styles.topTableContainer} data-testid="topTable">
-        <AutoSizer style={{ width: '100%', height }}>
+        <AutoSizer style={{ width: '100%' }}>
           {({ width, height }) => {
             if (width < 3 || height < 3) {
               return null;
@@ -319,21 +310,14 @@ function ActionCell(props: ActionCellProps) {
   );
 }
 
-const getStyles = (theme: GrafanaTheme2, height: number) => {
+const getStyles = (theme: GrafanaTheme2) => {
   return {
-    topTableContainer: css`
+    topTableContainer: css({
-      label: topTableContainer;
+      label: 'topTableContainer',
-      flex-grow: 1;
+      padding: theme.spacing(1),
-      flex-basis: 50%;
+      backgroundColor: theme.colors.background.secondary,
-      overflow: hidden;
+      height: '100%',
-      padding: ${theme.spacing(1)};
+    }),
-      background-color: ${theme.colors.background.secondary};
-      ${height
-        ? css`
-            min-height: ${height}px;
-          `
-        : ''}
-    `,
   };
 };