Skip to content
Snippets Groups Projects

#3472089: Improve/Refactor replaceUUIDsAndUpdateModel() and insertMultipleNodes()

Open #3472089: Improve/Refactor replaceUUIDsAndUpdateModel() and insertMultipleNodes()
9 unresolved threads
9 unresolved threads

Closes #3472089

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
1 1 // cspell:ignore idontexist
2 import { isChildNode } from '../../../../../ui/src/features/layout/layoutUtils';
2 import {
3 isChildNode,
4 replaceUUIDsAndUpdateModel,
5 insertMultipleNodesUtil,
6 } from '../../../../../ui/src/features/layout/layoutUtils';
7 import _ from 'lodash';
3 8
4 9 let layout;
5 before('Load fixture', function () {
10 before('Load fixtures', function () {
11 // Load layout fixture
  • 126 },
    127 ],
    128 };
    129
    130 const to = [1]; // Inserting at index 1
    131 const model = {}; // Your initial model, if needed
    132
    133 // Call the utility function
    134 const { newLayout, updatedModel } = insertMultipleNodesUtil(
    135 layout,
    136 newNodes,
    137 to,
    138 model,
    139 );
    140
    141 // Assertions
  • 115 };
    116
    117 const newNodes = {
    118 children: [
    119 {
    120 nodeType: 'component',
    121 type: 'newComponent1',
    122 },
    123 {
    124 nodeType: 'component',
    125 type: 'newComponent2',
    126 },
    127 ],
    128 };
    129
    130 const to = [1]; // Inserting at index 1
  • DEEPAK MISHRA added 5 commits

    added 5 commits

    • 57985bb6...be472031 - 4 commits from branch project:0.x
    • 8e23766e - Merge branch experience_builder:0.x into 3472089-refactor-replaceUUIDsAndUpdateModel

    Compare with previous version

  • DEEPAK MISHRA added 1 commit

    added 1 commit

    Compare with previous version

  • DEEPAK MISHRA added 1 commit

    added 1 commit

    Compare with previous version

  • 74 }
    75
    76 /**
    77 * Replace UUIDs in a layout node and its corresponding model.
    78 * @param node - The layout node to update.
    79 * @param model - The corresponding model to update.
    80 * @returns An updated model and a updated state.
    81 */
    82 export function replaceUUIDsAndUpdateModel(
    83 node: LayoutNode,
    84 model: ComponentModels,
    85 ): {
    86 updatedNode: LayoutNode;
    87 updatedModel: ComponentModels;
    88 } {
    89 var oldToNewUUIDMap: Record<string, string> = {};
  • Jesse Baker
  • We should not be using var anywhere so I'm not sure why this was changed from let/const to var all over.

    I think moving these functions out to the utils file and adding tests is a good step forward but I think the intention was to try and reduce the complexity and improve the readability of these functions more than just moving them to another place and making minor changes.

  • Jesse Baker requested changes

    requested changes

  • hemanshu_412 added 1 commit

    added 1 commit

    • 7e7bfbc5 - Added the Requested Changes.

    Compare with previous version

  • 38 46 }
    39 47 }
    40 48
    49 export function insertMultipleNodesUtil(
    50 layout: LayoutNode,
    51 newNodes: LayoutNode,
    52 to: number[],
    53 model: ComponentModels,
    54 ): { newLayout: LayoutNode; updatedModel: ComponentModels } {
    55 let updatedModel: ComponentModels = { ...model };
    56 const nodesToInsert: LayoutNode[] = _.cloneDeep(newNodes.children);
    57 let newLayout: LayoutNode = _.cloneDeep(layout);
    58
    59 for (let i = nodesToInsert.length - 1; i >= 0; i--) {
  • 69
    70 /**
    71 * Replace UUIDs in a layout node and its corresponding model.
    72 * @param node - The layout node to update.
    73 * @param model - The corresponding model to update.
    74 * @returns An updated model and a updated state.
    75 */
    76 export function replaceUUIDsAndUpdateModel(
    77 node: LayoutNode,
    78 model: ComponentModels,
    79 ): {
    80 updatedNode: LayoutNode;
    81 updatedModel: ComponentModels;
    82 } {
    83 let oldToNewUUIDMap: Record<string, string> = {};
    84 let updatedModel: ComponentModels = {};
    • Comment on lines +83 to +84

      Both of these can be const as they were before this was moved to layoutUtils.ts.

      Suggested change
      83 let oldToNewUUIDMap: Record<string, string> = {};
      84 let updatedModel: ComponentModels = {};
      83 const oldToNewUUIDMap: Record<string, string> = {};
      84 const updatedModel: ComponentModels = {};
    • Please register or sign in to reply
  • 72 * @param node - The layout node to update.
    73 * @param model - The corresponding model to update.
    74 * @returns An updated model and a updated state.
    75 */
    76 export function replaceUUIDsAndUpdateModel(
    77 node: LayoutNode,
    78 model: ComponentModels,
    79 ): {
    80 updatedNode: LayoutNode;
    81 updatedModel: ComponentModels;
    82 } {
    83 let oldToNewUUIDMap: Record<string, string> = {};
    84 let updatedModel: ComponentModels = {};
    85
    86 function replaceUUIDs(node: LayoutNode): LayoutNode {
    87 let newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() });
    • Suggested change
      87 let newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() });
      87 const newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() });
    • Please register or sign in to reply
  • 84 let updatedModel: ComponentModels = {};
    85
    86 function replaceUUIDs(node: LayoutNode): LayoutNode {
    87 let newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() });
    88
    89 oldToNewUUIDMap[node.uuid] = newNode.uuid;
    90
    91 if (newNode.children) {
    92 newNode.children = newNode.children.map(replaceUUIDs);
    93 }
    94 return newNode;
    95 }
    96
    97 let updatedNode = replaceUUIDs(node);
    98
    99 for (let oldUUID in model) {
  • 86 function replaceUUIDs(node: LayoutNode): LayoutNode {
    87 let newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() });
    88
    89 oldToNewUUIDMap[node.uuid] = newNode.uuid;
    90
    91 if (newNode.children) {
    92 newNode.children = newNode.children.map(replaceUUIDs);
    93 }
    94 return newNode;
    95 }
    96
    97 let updatedNode = replaceUUIDs(node);
    98
    99 for (let oldUUID in model) {
    100 if (Object.prototype.hasOwnProperty.call(model, oldUUID)) {
    101 let newUUID = oldToNewUUIDMap[oldUUID];
  • Fantastic work on adding the tests, that's really helpful.

    I do think that moving these function into the layoutUtils.ts is a good start but the actual functions themselves are no different than they were before.

    In moving them, a number of consts were changed to let which I think is a regression so they should be changed back. Also some of the comments didn't make it across which I also think is step backwards.

    Now, with tests in place it would be nice to see if these can be improved for better readability and/or performance.

  • Jesse Baker requested changes

    requested changes

  • Please register or sign in to reply
    Loading