From b48e1f897efec6cf1b089daf32ca5653e32e299d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Torkel=20=C3=96degaard?= <torkel@grafana.com>
Date: Tue, 30 Jan 2024 16:07:24 +0100
Subject: [PATCH] Dashboard: Improve diff styling (#81509)

* Dashboard: Improve diff styling

* Update public/app/features/dashboard-scene/settings/version-history/DiffGroup.tsx

Co-authored-by: Dominik Prokop <dominik.prokop@grafana.com>

* Fix

* Update

* Update

---------

Co-authored-by: Dominik Prokop <dominik.prokop@grafana.com>
---
 .betterer.results                             |  3 -
 .../saving/SaveDashboardDrawer.test.tsx       |  2 +-
 .../settings/version-history/DiffGroup.tsx    | 10 +-
 .../settings/version-history/DiffTitle.tsx    | 11 ++-
 .../settings/version-history/DiffValues.tsx   |  4 +-
 .../VersionHistoryComparison.tsx              | 95 ++++++++++---------
 .../version-history/VersionHistoryHeader.tsx  |  2 +-
 .../SaveDashboard/SaveDashboardDiff.tsx       | 28 +++---
 .../VersionHistoryComparison.tsx              | 93 +++++++++---------
 9 files changed, 120 insertions(+), 128 deletions(-)

diff --git a/.betterer.results b/.betterer.results
index 17ab6ecef9a..4dfb3a57bb4 100644
--- a/.betterer.results
+++ b/.betterer.results
@@ -2658,9 +2658,6 @@ exports[`better eslint`] = {
       [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"],
       [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "1"]
     ],
-    "public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx:5381": [
-      [0, 0, 0, "Styles should be written using objects.", "0"]
-    ],
     "public/app/features/dashboard/components/SaveDashboard/SaveDashboardErrorProxy.tsx:5381": [
       [0, 0, 0, "Styles should be written using objects.", "0"],
       [0, 0, 0, "Styles should be written using objects.", "1"],
diff --git a/public/app/features/dashboard-scene/saving/SaveDashboardDrawer.test.tsx b/public/app/features/dashboard-scene/saving/SaveDashboardDrawer.test.tsx
index 71b525ea389..a015b2a8964 100644
--- a/public/app/features/dashboard-scene/saving/SaveDashboardDrawer.test.tsx
+++ b/public/app/features/dashboard-scene/saving/SaveDashboardDrawer.test.tsx
@@ -77,7 +77,7 @@ describe('SaveDashboardDrawer', () => {
 
       await userEvent.click(await screen.findByLabelText('Tab Changes'));
 
-      expect(await screen.findByText('JSON Model')).toBeInTheDocument();
+      expect(await screen.findByText('Full JSON diff')).toBeInTheDocument();
     });
 
     it('Can save', async () => {
diff --git a/public/app/features/dashboard-scene/settings/version-history/DiffGroup.tsx b/public/app/features/dashboard-scene/settings/version-history/DiffGroup.tsx
index 5d56d58ac53..cbded5fd9ce 100644
--- a/public/app/features/dashboard-scene/settings/version-history/DiffGroup.tsx
+++ b/public/app/features/dashboard-scene/settings/version-history/DiffGroup.tsx
@@ -42,16 +42,14 @@ export const DiffGroup = ({ diffs, title }: DiffGroupProps) => {
 };
 
 const getStyles = (theme: GrafanaTheme2) => ({
-  container: css({
-    backgroundColor: theme.colors.background.secondary,
-    fontSize: theme.typography.h6.fontSize,
-    marginBottom: theme.spacing(2),
-    padding: theme.spacing(2),
-  }),
+  container: css({}),
   list: css({
     marginLeft: theme.spacing(4),
   }),
   listItem: css({
     marginBottom: theme.spacing(1),
+    '&:last-child': {
+      marginBottom: 0,
+    },
   }),
 });
diff --git a/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx b/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx
index db0fc5c7390..3716d3534e1 100644
--- a/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx
+++ b/public/app/features/dashboard-scene/settings/version-history/DiffTitle.tsx
@@ -19,13 +19,14 @@ export const DiffTitle = ({ diff, title }: DiffTitleProps) => {
 
   return diff ? (
     <>
-      <Icon type="mono" name="circle" className={styles[diff.op]} /> <span className={styles.embolden}>{title}</span>{' '}
-      <span>{getDiffText(diff, diff.path.length > 1)}</span> <DiffValues diff={diff} />
+      <Icon type="mono" name="circle" className={styles[diff.op]} size="xs" />{' '}
+      <span className={styles.embolden}>{title}</span> <span>{getDiffText(diff, diff.path.length > 1)}</span>{' '}
+      <DiffValues diff={diff} />
     </>
   ) : (
     <div className={styles.withoutDiff}>
-      <Icon type="mono" name="circle" className={styles.replace} /> <span className={styles.embolden}>{title}</span>{' '}
-      <span>{getDiffText(replaceDiff, false)}</span>
+      <Icon type="mono" name="circle" className={styles.replace} size="xs" />{' '}
+      <span className={styles.embolden}>{title}</span> <span>{getDiffText(replaceDiff, false)}</span>
     </div>
   );
 };
@@ -56,6 +57,6 @@ const getDiffTitleStyles = (theme: GrafanaTheme2) => ({
     color: theme.colors.success.main,
   }),
   withoutDiff: css({
-    marginBottom: theme.spacing(2),
+    marginBottom: theme.spacing(1),
   }),
 });
diff --git a/public/app/features/dashboard-scene/settings/version-history/DiffValues.tsx b/public/app/features/dashboard-scene/settings/version-history/DiffValues.tsx
index d17976ee12d..c9ff1247f90 100644
--- a/public/app/features/dashboard-scene/settings/version-history/DiffValues.tsx
+++ b/public/app/features/dashboard-scene/settings/version-history/DiffValues.tsx
@@ -32,6 +32,6 @@ const getStyles = (theme: GrafanaTheme2) =>
     borderRadius: theme.shape.radius.default,
     color: theme.colors.text.primary,
     fontSize: theme.typography.body.fontSize,
-    margin: `0 ${theme.spacing(0.5)}`,
-    padding: theme.spacing(0.5, 1),
+    margin: theme.spacing(0, 0.5),
+    padding: theme.spacing(0.25, 0.5),
   });
diff --git a/public/app/features/dashboard-scene/settings/version-history/VersionHistoryComparison.tsx b/public/app/features/dashboard-scene/settings/version-history/VersionHistoryComparison.tsx
index 3cc0241d7c3..7faaaa587b5 100644
--- a/public/app/features/dashboard-scene/settings/version-history/VersionHistoryComparison.tsx
+++ b/public/app/features/dashboard-scene/settings/version-history/VersionHistoryComparison.tsx
@@ -2,7 +2,7 @@ import { css, cx } from '@emotion/css';
 import React from 'react';
 
 import { GrafanaTheme2 } from '@grafana/data';
-import { Button, ModalsController, CollapsableSection, HorizontalGroup, useStyles2 } from '@grafana/ui';
+import { Button, ModalsController, CollapsableSection, useStyles2, Stack, Icon, Box } from '@grafana/ui';
 
 import { DecoratedRevisionModel } from '../VersionsEditView';
 
@@ -24,56 +24,57 @@ export const VersionHistoryComparison = ({ baseInfo, newInfo, diffData, isNewLat
   const styles = useStyles2(getStyles);
 
   return (
-    <div>
-      <div className={styles.spacer}>
-        <HorizontalGroup justify="space-between" align="center">
-          <div>
-            <p className={styles.versionInfo}>
-              <strong>Version {newInfo.version}</strong> updated by {newInfo.createdBy} {newInfo.ageString} -{' '}
-              {newInfo.message}
-            </p>
-            <p className={cx(styles.versionInfo, styles.noMarginBottom)}>
-              <strong>Version {baseInfo.version}</strong> updated by {baseInfo.createdBy} {baseInfo.ageString} -{' '}
-              {baseInfo.message}
-            </p>
-          </div>
-          {isNewLatest && (
-            <ModalsController>
-              {({ showModal, hideModal }) => (
-                <Button
-                  variant="destructive"
-                  icon="history"
-                  onClick={() => {
-                    showModal(RevertDashboardModal, {
-                      version: baseInfo,
-                      onRestore,
-                      hideModal,
-                    });
-                  }}
-                >
-                  Restore to version {baseInfo.version}
-                </Button>
-              )}
-            </ModalsController>
-          )}
-        </HorizontalGroup>
-      </div>
-      <div className={styles.spacer}>
-        {Object.entries(diff).map(([key, diffs]) => (
-          <DiffGroup diffs={diffs} key={key} title={key} />
-        ))}
-      </div>
-      <CollapsableSection isOpen={false} label="View JSON Diff">
-        <DiffViewer oldValue={JSON.stringify(diffData.lhs, null, 2)} newValue={JSON.stringify(diffData.rhs, null, 2)} />
-      </CollapsableSection>
-    </div>
+    <Stack direction="column" gap={1}>
+      <Stack justifyContent="space-between" alignItems="center">
+        <Stack alignItems="center">
+          <span className={cx(styles.versionInfo, styles.noMarginBottom)}>
+            <strong>Version {baseInfo.version}</strong> updated by {baseInfo.createdBy} {baseInfo.ageString}
+            {baseInfo.message}
+          </span>
+          <Icon name="arrow-right" size="sm" />
+          <span className={styles.versionInfo}>
+            <strong>Version {newInfo.version}</strong> updated by {newInfo.createdBy} {newInfo.ageString}
+            {newInfo.message}
+          </span>
+        </Stack>
+        {isNewLatest && (
+          <ModalsController>
+            {({ showModal, hideModal }) => (
+              <Button
+                variant="destructive"
+                icon="history"
+                onClick={() => {
+                  showModal(RevertDashboardModal, {
+                    version: baseInfo,
+                    onRestore,
+                    hideModal,
+                  });
+                }}
+              >
+                Restore to version {baseInfo.version}
+              </Button>
+            )}
+          </ModalsController>
+        )}
+      </Stack>
+
+      {Object.entries(diff).map(([key, diffs]) => (
+        <DiffGroup diffs={diffs} key={key} title={key} />
+      ))}
+
+      <Box paddingTop={2}>
+        <CollapsableSection isOpen={false} label="View JSON Diff">
+          <DiffViewer
+            oldValue={JSON.stringify(diffData.lhs, null, 2)}
+            newValue={JSON.stringify(diffData.rhs, null, 2)}
+          />
+        </CollapsableSection>
+      </Box>
+    </Stack>
   );
 };
 
 const getStyles = (theme: GrafanaTheme2) => ({
-  spacer: css({
-    marginBottom: theme.spacing(4),
-  }),
   versionInfo: css({
     color: theme.colors.text.secondary,
     fontSize: theme.typography.bodySmall.fontSize,
diff --git a/public/app/features/dashboard-scene/settings/version-history/VersionHistoryHeader.tsx b/public/app/features/dashboard-scene/settings/version-history/VersionHistoryHeader.tsx
index 721b7b6dade..6c3738f9057 100644
--- a/public/app/features/dashboard-scene/settings/version-history/VersionHistoryHeader.tsx
+++ b/public/app/features/dashboard-scene/settings/version-history/VersionHistoryHeader.tsx
@@ -36,6 +36,6 @@ const getStyles = (theme: GrafanaTheme2) => ({
     fontSize: theme.typography.h3.fontSize,
     display: 'flex',
     gap: theme.spacing(2),
-    marginBottom: theme.spacing(3),
+    marginBottom: theme.spacing(2),
   }),
 });
diff --git a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx
index 850804f19f1..97c0462df76 100644
--- a/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx
+++ b/public/app/features/dashboard/components/SaveDashboard/SaveDashboardDiff.tsx
@@ -1,9 +1,7 @@
-import { css } from '@emotion/css';
 import React, { ReactElement } from 'react';
 import { useAsync } from 'react-use';
 
-import { GrafanaTheme2 } from '@grafana/data';
-import { Spinner, useStyles2 } from '@grafana/ui';
+import { Box, Spinner, Stack } from '@grafana/ui';
 import { Diffs } from 'app/features/dashboard-scene/settings/version-history/utils';
 
 import { DiffGroup } from '../../../dashboard-scene/settings/version-history/DiffGroup';
@@ -18,7 +16,6 @@ interface SaveDashboardDiffProps {
 }
 
 export const SaveDashboardDiff = ({ diff, oldValue, newValue }: SaveDashboardDiffProps) => {
-  const styles = useStyles2(getStyles);
   const loader = useAsync(async () => {
     const oldJSON = JSON.stringify(oldValue ?? {}, null, 2);
     const newJSON = JSON.stringify(newValue ?? {}, null, 2);
@@ -27,6 +24,7 @@ export const SaveDashboardDiff = ({ diff, oldValue, newValue }: SaveDashboardDif
     let schemaChange: ReactElement | undefined = undefined;
     const diffs: ReactElement[] = [];
     let count = 0;
+
     if (diff) {
       for (const [key, changes] of Object.entries(diff)) {
         // this takes a long time for large diffs (so this is async)
@@ -39,6 +37,7 @@ export const SaveDashboardDiff = ({ diff, oldValue, newValue }: SaveDashboardDif
         count += changes.length;
       }
     }
+
     return {
       schemaChange,
       diffs,
@@ -58,19 +57,14 @@ export const SaveDashboardDiff = ({ diff, oldValue, newValue }: SaveDashboardDif
   }
 
   return (
-    <div>
-      {value.schemaChange && <div className={styles.spacer}>{value.schemaChange}</div>}
+    <Stack direction="column" gap={1}>
+      {value.schemaChange && value.schemaChange}
+      {value.showDiffs && value.diffs}
 
-      {value.showDiffs && <div className={styles.spacer}>{value.diffs}</div>}
-
-      <h4>JSON Model</h4>
-      {value.jsonView}
-    </div>
+      <Box paddingTop={2}>
+        <h4>Full JSON diff</h4>
+        {value.jsonView}
+      </Box>
+    </Stack>
   );
 };
-
-const getStyles = (theme: GrafanaTheme2) => ({
-  spacer: css`
-    margin-bottom: ${theme.v1.spacing.xl};
-  `,
-});
diff --git a/public/app/features/dashboard/components/VersionHistory/VersionHistoryComparison.tsx b/public/app/features/dashboard/components/VersionHistory/VersionHistoryComparison.tsx
index f2ecdd2593b..0dab8cccf2c 100644
--- a/public/app/features/dashboard/components/VersionHistory/VersionHistoryComparison.tsx
+++ b/public/app/features/dashboard/components/VersionHistory/VersionHistoryComparison.tsx
@@ -2,7 +2,7 @@ import { css, cx } from '@emotion/css';
 import React from 'react';
 
 import { GrafanaTheme2 } from '@grafana/data';
-import { Button, ModalsController, CollapsableSection, HorizontalGroup, useStyles2 } from '@grafana/ui';
+import { Button, ModalsController, CollapsableSection, useStyles2, Stack, Icon, Box } from '@grafana/ui';
 import { DiffGroup } from 'app/features/dashboard-scene/settings/version-history/DiffGroup';
 import { DiffViewer } from 'app/features/dashboard-scene/settings/version-history/DiffViewer';
 import { jsonDiff } from 'app/features/dashboard-scene/settings/version-history/utils';
@@ -23,55 +23,56 @@ export const VersionHistoryComparison = ({ baseInfo, newInfo, diffData, isNewLat
   const styles = useStyles2(getStyles);
 
   return (
-    <div>
-      <div className={styles.spacer}>
-        <HorizontalGroup justify="space-between" align="center">
-          <div>
-            <p className={styles.versionInfo}>
-              <strong>Version {newInfo.version}</strong> updated by {newInfo.createdBy} {newInfo.ageString} -{' '}
-              {newInfo.message}
-            </p>
-            <p className={cx(styles.versionInfo, styles.noMarginBottom)}>
-              <strong>Version {baseInfo.version}</strong> updated by {baseInfo.createdBy} {baseInfo.ageString} -{' '}
-              {baseInfo.message}
-            </p>
-          </div>
-          {isNewLatest && (
-            <ModalsController>
-              {({ showModal, hideModal }) => (
-                <Button
-                  variant="destructive"
-                  icon="history"
-                  onClick={() => {
-                    showModal(RevertDashboardModal, {
-                      version: baseInfo.version,
-                      hideModal,
-                    });
-                  }}
-                >
-                  Restore to version {baseInfo.version}
-                </Button>
-              )}
-            </ModalsController>
-          )}
-        </HorizontalGroup>
-      </div>
-      <div className={styles.spacer}>
-        {Object.entries(diff).map(([key, diffs]) => (
-          <DiffGroup diffs={diffs} key={key} title={key} />
-        ))}
-      </div>
-      <CollapsableSection isOpen={false} label="View JSON Diff">
-        <DiffViewer oldValue={JSON.stringify(diffData.lhs, null, 2)} newValue={JSON.stringify(diffData.rhs, null, 2)} />
-      </CollapsableSection>
-    </div>
+    <Stack direction="column" gap={1}>
+      <Stack justifyContent="space-between" alignItems="center">
+        <Stack alignItems="center">
+          <span className={cx(styles.versionInfo, styles.noMarginBottom)}>
+            <strong>Version {baseInfo.version}</strong> updated by {baseInfo.createdBy} {baseInfo.ageString}
+            {baseInfo.message}
+          </span>
+          <Icon name="arrow-right" size="sm" />
+          <span className={styles.versionInfo}>
+            <strong>Version {newInfo.version}</strong> updated by {newInfo.createdBy} {newInfo.ageString}
+            {newInfo.message}
+          </span>
+        </Stack>
+        {isNewLatest && (
+          <ModalsController>
+            {({ showModal, hideModal }) => (
+              <Button
+                variant="destructive"
+                icon="history"
+                onClick={() => {
+                  showModal(RevertDashboardModal, {
+                    version: baseInfo.version,
+                    hideModal,
+                  });
+                }}
+              >
+                Restore to version {baseInfo.version}
+              </Button>
+            )}
+          </ModalsController>
+        )}
+      </Stack>
+
+      {Object.entries(diff).map(([key, diffs]) => (
+        <DiffGroup diffs={diffs} key={key} title={key} />
+      ))}
+
+      <Box paddingTop={2}>
+        <CollapsableSection isOpen={false} label="View JSON Diff">
+          <DiffViewer
+            oldValue={JSON.stringify(diffData.lhs, null, 2)}
+            newValue={JSON.stringify(diffData.rhs, null, 2)}
+          />
+        </CollapsableSection>
+      </Box>
+    </Stack>
   );
 };
 
 const getStyles = (theme: GrafanaTheme2) => ({
-  spacer: css({
-    marginBottom: theme.spacing(4),
-  }),
   versionInfo: css({
     color: theme.colors.text.secondary,
     fontSize: theme.typography.bodySmall.fontSize,