add allow list of props for tidier formatted op summaries

theopensystemslab · May 6, 2024 · adca0b0 · adca0b0
1 parent 750d73e
commit adca0b0
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 37 deletions.
diff --git a/editor.planx.uk/src/@planx/components/shared/Preview/SimpleExpand.tsx b/editor.planx.uk/src/@planx/components/shared/Preview/SimpleExpand.tsx
@@ -6,11 +6,13 @@ import React, { PropsWithChildren } from "react";
 import { FONT_WEIGHT_SEMI_BOLD } from "theme";
 import Caret from "ui/icons/Caret";
 
-const StyledButton = styled(Button)(() => ({
+const StyledButton = styled(Button, {
+  shouldForwardProp: (prop) => prop !== "lightFontStyle",
+})<{ lightFontStyle: boolean }>(({ theme, lightFontStyle }) => ({
   boxShadow: "none",
-  color: "black",
-  fontSize: "1.125rem",
-  fontWeight: FONT_WEIGHT_SEMI_BOLD,
+  color: lightFontStyle ? theme.palette.grey[600] : "black",
+  fontSize: lightFontStyle ? "1rem" : "1.125rem",
+  fontWeight: lightFontStyle ? "inherit" : FONT_WEIGHT_SEMI_BOLD,
   width: "100%",
 }));
 
@@ -20,12 +22,14 @@ interface Props {
     closed: string;
   };
   id: string;
+  lightFontStyle?: boolean;
 }
 
 const SimpleExpand: React.FC<PropsWithChildren<Props>> = ({
   children,
   buttonText,
   id,
+  lightFontStyle,
 }) => {
   const [show, setShow] = React.useState(false);
   return (
@@ -35,6 +39,7 @@ const SimpleExpand: React.FC<PropsWithChildren<Props>> = ({
           onClick={() => setShow(!show)}
           aria-expanded={show}
           aria-controls={id}
+          lightFontStyle={lightFontStyle || false}
         >
           {show ? buttonText.closed : buttonText.open}
           <Caret

diff --git a/editor.planx.uk/src/@planx/graph/__tests__/formatOps.test.ts b/editor.planx.uk/src/@planx/graph/__tests__/formatOps.test.ts
@@ -1,4 +1,4 @@
-import { formatOps,Graph } from "../index";
+import { formatOps, Graph } from "../index";
 
 describe("Update operations", () => {
   test("Updating a single property of a node", () => {
@@ -118,12 +118,8 @@ describe("Insert operations", () => {
     ]);
   });
 
-  test("Adding a new child and data property to an existing node", () => {
+  test("Adding a new child to an existing node", () => {
     const ops = [
-      {
-        p: ["FW5G3EMBI3", "data", "description"],
-        oi: "<p>Fruits contain seeds and come from the flower of a plant</p>",
-      },
       {
         p: ["FW5G3EMBI3", "edges"],
         oi: ["WDwUTbF7Gq", "SO5XbLwSYp", "xTBfSd1Tjy", "zzQAMXexRj"],
@@ -141,11 +137,23 @@ describe("Insert operations", () => {
     ];
 
     expect(formatOps(flowWithChecklist, ops)).toEqual([
-      'Added Checklist description "<p>Fruits contain seeds and come from the flower of a plant</p>"',
       "Updated order of Checklist edges",
       'Added Answer "Strawberry"',
     ]);
   });
+
+  test("Adding a data property to an existing node that is not in `allowProps`", () => {
+    const ops = [
+      {
+        p: ["FW5G3EMBI3", "data", "description"],
+        oi: "<p>Fruits contain seeds and come from the flower of a plant</p>",
+      },
+    ];
+
+    expect(formatOps(flowWithChecklist, ops)).toEqual([
+      "Added Checklist description", // only shows "description", not content
+    ]);
+  });
 });
 
 describe("Remove operations", () => {

diff --git a/editor.planx.uk/src/@planx/graph/index.ts b/editor.planx.uk/src/@planx/graph/index.ts
@@ -507,6 +507,9 @@ export const sortIdsDepthFirst =
 export const formatOps = (graph: Graph, ops: Array<OT.Op>): string[] => {
   const output: string[] = [];
 
+  // Only show full change description for simple props, omit complex or long ones like `moreInfo` etc
+  const allowProps = ["title", "text", "fn", "val"];
+
   // Updating a node or its properties (update = delete + insert)
   const handleUpdate = (node: Node, op: OT.Object.Replace) => {
     if (op.od?.type && op.oi?.type) {
@@ -516,21 +519,28 @@ export const formatOps = (graph: Graph, ops: Array<OT.Op>): string[] => {
           op.od.data?.text ||
           op.od.data?.content ||
           op.od.data?.fn ||
+          op.od.data?.val ||
           op.od.data?.flowId
         }" with ${TYPES[op.oi.type]} "${
           op.oi.data?.title ||
           op.oi.data?.text ||
           op.oi.data?.content ||
           op.oi.data?.fn ||
+          op.oi.data?.val ||
           op.od.data?.flowId
         }"`,
       );
     } else if (op.p.includes("data")) {
-      output.push(
-        `Updated ${node?.type ? TYPES[node.type] : "node"} ${op.p?.[2]} from "${
-          op.od
-        }" to "${op.oi}"`,
-      );
+      if (allowProps.includes(`${op.p?.[2]}`)) {
+        output.push(
+          `Updated ${node?.type ? TYPES[node.type] : "node"} ${op
+            .p?.[2]} from "${op.od}" to "${op.oi}"`,
+        );
+      } else {
+        output.push(
+          `Updated ${node?.type ? TYPES[node.type] : "node"} ${op.p?.[2]}`,
+        );
+      }
     } else if (op.p.includes("edges")) {
       output.push(
         `Updated order of ${node?.type ? TYPES[node.type] : "graph"} edges`,
@@ -560,11 +570,17 @@ export const formatOps = (graph: Graph, ops: Array<OT.Op>): string[] => {
         }"`,
       );
     } else if (op.p.includes("data")) {
-      output.push(
-        `Added ${node?.type ? TYPES[node?.type] : "node"} ${op.p?.[2]} "${
-          op.oi
-        }"`,
-      );
+      if (allowProps.includes(`${op.p?.[2]}`)) {
+        output.push(
+          `Added ${node?.type ? TYPES[node?.type] : "node"} ${op.p?.[2]} "${
+            op.oi
+          }"`,
+        );
+      } else {
+        output.push(
+          `Added ${node?.type ? TYPES[node?.type] : "node"} ${op.p?.[2]}`,
+        );
+      }
     } else if (op.p.includes("edges")) {
       const node = graph[op.oi?.[0]];
       output.push(`Added ${node?.type ? TYPES[node.type] : "node"} to branch`);
@@ -584,11 +600,17 @@ export const formatOps = (graph: Graph, ops: Array<OT.Op>): string[] => {
         }"`,
       );
     } else if (op.p.includes("data")) {
-      output.push(
-        `Removed ${node?.type ? TYPES[node.type] : "node"} ${op.p?.[2]} "${
-          op.od
-        }"`,
-      );
+      if (allowProps.includes(`${op.p?.[2]}`)) {
+        output.push(
+          `Removed ${node?.type ? TYPES[node.type] : "node"} ${op.p?.[2]} "${
+            op.od
+          }"`,
+        );
+      } else {
+        output.push(
+          `Removed ${node?.type ? TYPES[node.type] : "node"} ${op.p?.[2]}`,
+        );
+      }
     } else if (op.p.includes("edges")) {
       const node = graph[op.od?.[0]];
       output.push(

diff --git a/editor.planx.uk/src/pages/FlowEditor/index.tsx b/editor.planx.uk/src/pages/FlowEditor/index.tsx
@@ -15,6 +15,7 @@ import IconButton from "@mui/material/IconButton";
 import Link from "@mui/material/Link";
 import { styled } from "@mui/material/styles";
 import Typography from "@mui/material/Typography";
+import SimpleExpand from "@planx/components/shared/Preview/SimpleExpand";
 import { formatOps } from "@planx/graph";
 import { OT } from "@planx/graph/types";
 import DelayedLoadingIndicator from "components/DelayedLoadingIndicator";
@@ -139,6 +140,8 @@ export const LastEdited = () => {
 };
 
 export const EditHistory = () => {
+  const OPS_TO_DISPLAY = 5;
+
   const [focusedOpIndex, setFocusedOpIndex] = useState<number | undefined>(
     undefined,
   );
@@ -278,18 +281,47 @@ export const EditHistory = () => {
                   )}
                 </Box>
                 {op.data && (
-                  <Typography
-                    variant="body2"
-                    component="ul"
-                    padding={2}
-                    color={inFocus(i) ? "GrayText" : "inherit"}
-                  >
-                    {[...new Set(formatOps(flow, op.data))].map(
-                      (formattedOp, i) => (
-                        <li key={i}>{formattedOp}</li>
-                      ),
+                  <>
+                    <Typography
+                      variant="body2"
+                      component="ul"
+                      padding={2}
+                      color={inFocus(i) ? "GrayText" : "inherit"}
+                    >
+                      {[...new Set(formatOps(flow, op.data))]
+                        .slice(0, OPS_TO_DISPLAY)
+                        .map((formattedOp, i) => (
+                          <li key={i}>{formattedOp}</li>
+                        ))}
+                    </Typography>
+                    {[...new Set(formatOps(flow, op.data))].length >
+                      OPS_TO_DISPLAY && (
+                      <SimpleExpand
+                        id="edits-overflow"
+                        buttonText={{
+                          open: `Show ${
+                            [...new Set(formatOps(flow, op.data))].length -
+                            OPS_TO_DISPLAY
+                          } more`,
+                          closed: "Show less",
+                        }}
+                        lightFontStyle={true}
+                      >
+                        <Typography
+                          variant="body2"
+                          component="ul"
+                          padding={2}
+                          color={inFocus(i) ? "GrayText" : "inherit"}
+                        >
+                          {[...new Set(formatOps(flow, op.data))]
+                            .slice(OPS_TO_DISPLAY)
+                            .map((formattedOp, i) => (
+                              <li key={i}>{formattedOp}</li>
+                            ))}
+                        </Typography>
+                      </SimpleExpand>
                     )}
-                  </Typography>
+                  </>
                 )}
               </TimelineContent>
             </TimelineItem>