code review

LICENSE.md

@@ -1,6 +1,6 @@
 MIT License
 
-Copyright (c) 2022 Tibo Stroo
+Copyright (c) 2022 KNowledge on Web Scale
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal

package.json

@@ -10,12 +10,12 @@
     "ajv": "^8.11.0",
     "ajv-errors": "^3.0.0",
     "bootstrap": "^5.2.0",
-    "dagre": "latest",
+    "dagre": "^0.8.5",
     "js-yaml": "^4.1.0",
     "react": "^18.2.0",
-    "react-bootstrap": "^2.4.0",
+    "react-bootstrap": "^2.5.0",
     "react-dom": "^18.2.0",
-    "react-flow-renderer": "^10.3.11",
+    "react-flow-renderer": "^10.3.12",
     "react-scripts": "5.0.1"
   },
   "scripts": {

src/App.js

@@ -42,10 +42,11 @@ const EdgesFlow = () => {
 
 
             <div style={{
-                width: window.innerWidth * 0.96,
+                width: window.innerWidth * 0.48,
                 height: window.innerHeight * 0.96,
                 border: "solid 1px black",
-                margin: "10px auto 10px auto"
+                margin: "10px auto 10px auto",
+                display: "inline-block"
             }}>
                 <ReactFlow
                     nodes={nodes}

src/CustomEdge.js

@@ -1,6 +1,7 @@
 import React from 'react';
 import { getBezierPath, getMarkerEnd } from 'react-flow-renderer';
 
+// TODO I don't think this file is being used? It's in the wrong location in any case
 export default function CustomEdge({
                                        id,
                                        sourceX,

src/custom/editors/EditorArea.js

@@ -4,6 +4,10 @@ import dagre from "dagre";
 import YAML from "js-yaml";
 import {useState} from "react";
 
+// TODO make this cleaner, something like below, which abstracts away all the example handling you're now doing in a UI component
+/*
+import examples from "../../data/examples"
+*/
 import {
     edgesJSON as edgesJSON1,
     globalDefaultsJSON as globalDefaultsJSON1,
@@ -55,6 +59,7 @@ const EditorArea = ({setNodes, setEdges}) => {
     const [errorMessages, setErrorMessages] = useState([]);
 
 
+    // TODO see previous todo: this should not be here anymore
     const examples = [
         [globalDefaultsJSON1, nodesJSON1, edgesJSON1], [globalDefaultsJSON2, nodesJSON2, edgesJSON2],
         [globalDefaultsJSON3, nodesJSON3, edgesJSON3], [globalDefaultsJSON4, nodesJSON4, edgesJSON4],
@@ -67,6 +72,7 @@ const EditorArea = ({setNodes, setEdges}) => {
         setErrorModalVisible(false)
     }
 
+    // TODO this is generic code
     function json2yaml(jsonData) {
         let yamlValue;
         try {
@@ -88,7 +94,7 @@ const EditorArea = ({setNodes, setEdges}) => {
         }
     }
 
-
+    // TODO this is a mix of non-UI code, and creates a dependency on 'examples', I'd split in 2 functions
     function loadExample(e, number) {
         e.preventDefault();
 
@@ -109,20 +115,21 @@ const EditorArea = ({setNodes, setEdges}) => {
         setEdgesData(edges);
     }
 
-    function changeLanguage(e) {
+    // TODO note how I changed the very vague `e` to a more descriptive variable name
+    function changeLanguage(eventKey) {
 
-        let newLang = e;
+        let newLang = eventKey;
 
         if (newLang === "yaml" && language === "json") {
             setGlobalDefaults(json2yaml(globalDefaults));
             setNodesData(json2yaml(nodesData));
             setEdgesData(json2yaml(edgesData));
-            setLanguage(e);
+            setLanguage(eventKey);
         } else if (newLang === "json" && language === "yaml") {
             setGlobalDefaults(yaml2json(globalDefaults));
             setNodesData(yaml2json(nodesData));
             setEdgesData(yaml2json(edgesData));
-            setLanguage(e);
+            setLanguage(eventKey);
         }
 
     }
@@ -198,9 +205,9 @@ const EditorArea = ({setNodes, setEdges}) => {
 
 
     return <>
-
+        {/* TODO Feels like this modal could be a separate component */}
         <Modal show={errorModalVisible} onHide={handleErrorPopUpClose}
-               scrollable={true}>
+            scrollable={true}>
             <Modal.Header closeButton>
                 <Modal.Title>{errorMessageTitle}</Modal.Title>
             </Modal.Header>
@@ -233,7 +240,7 @@ const EditorArea = ({setNodes, setEdges}) => {
         </Dropdown>
 
 
-        <div className="edit-area" id="global-default-editor">
+        <div className="edit-area" id="global-default-editor" style={{width:"49%", display: "inline-block"}}>
             <div className="code-editor resizable" style={{height: "200px"}}>
                 <h5>Global defaults editor</h5>
 
@@ -250,7 +257,9 @@ const EditorArea = ({setNodes, setEdges}) => {
                     <MyEditor language={language} data={nodesData} setData={setNodesData} modelName={"nodes-model"}
                               schema={nodeSchema}/>
                 </div>
-
+            </div>
+            <div className="d-flex resizable code-editor"
+                 style={{height: "350px"}}>
                 <div className="node-edge-editor">
                     <h5>Edge editor</h5>
                     <MyEditor language={language} data={edgesData} setData={setEdgesData} modelName={"edges-model"}

src/custom/editors/MyEditor.js

@@ -3,7 +3,8 @@ import {useState} from "react";
 
 const MyEditor = ({language, data, setData, modelName, schema}) => {
 
-
+    // TODO I get the heebie jeebies of variables starting with `my`, can you come up with more descriptive names?
+    // TODO same remark for this entire component :) Suggestion: CodeEditor
     const [myEditor, setMyEditor] = useState(null);
     const [myMonaco, setMyMonaco] = useState(null);
     const [myModel, setMyModel] = useState(null)

src/custom/editors/editorUtil.js

@@ -3,6 +3,7 @@
 // The values of this dict should be used in the JSON representation
 import {fixNodeGroups} from "./editorUtilPositioning";
 
+// TODO I have the feeling you should either have a 'constants.js' file, or rename this 'editorUtil.js' to 'configParsing.js' or smth
 export const GLOBAL_DEFAULT_KEY_VALUES = {
     "ANIMATED": {id: "animated", value: false, type: "boolean"},              // Standard animation supported by React Flow
     "ANIMATION": {id: "animation", type: "string"},          // Custom animation

src/custom/editors/editorUtilPositioning.js

@@ -30,6 +30,7 @@ export function getLayoutedElementsDagre(dagreGraph, nodes, edges, globalDefault
 
 }
 
+// TODO this function is only used for `editorUtil`: why not move it there so you can keep it private?
 export function fixNodeGroups(nodes) {
     // loop over nodes and store all groups
     // let groups = {"vgroups": new Set(), "hgroups": new Set()}

src/custom/node/SvgNode.js

@@ -5,7 +5,7 @@ import {Handle} from 'react-flow-renderer';
 import {NODE_KEYS} from "../editors/editorUtil";
 import {getShape} from "./nodeUtil";
 
-
+// TODO rename this file to better cover its contents
 export default memo(({data, isConnectable}) => {
 
     let width = data[NODE_KEYS.WIDTH.id];

src/custom/node/nodeUtil.js

@@ -2,7 +2,8 @@ import comunica from "../../assets/comunica.svg";
 import rmlio from "../../assets/rmlio.png";
 import solid from "../../assets/solid.svg";
 
-
+// TODO rename this file to better cover its contents
+// TODO why not put these svgs as actual files and import them? Makes this code a lot cleaner + you can edit the svgs in dedicated programs more easily
 export function getShape(shapeId, fill, stroke, strokeWidth) {
     const SHAPES = {
         "8-star":

src/data/data.js

@@ -1,5 +1,6 @@
 import {MarkerType} from "react-flow-renderer";
 
+// TODO I don't think this file is still being used
 export const nodesData = [
     {
         id: 'edges-1',