Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide/show child nodes of a branch #325

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8f8e65a
add: hidden field to mmpnode
sorenjohanson Jul 5, 2024
bdfcfc5
add: functionality to show/hide nodes
sorenjohanson Jul 5, 2024
c867398
add: tooltip and translations
sorenjohanson Jul 5, 2024
12ff66d
changed: use hasHiddenChildren instead
sorenjohanson Jul 10, 2024
7f5f35b
Merge branch 'main' into 105-hide-show-child-nodes-of-a-branch
sorenjohanson Jul 18, 2024
204583d
changed: use client-side hidden variable, add key function to dom nodes
sorenjohanson Jul 19, 2024
068e26a
fix: do not send websocket update when hiding nodes
sorenjohanson Jul 24, 2024
32d1e1e
fix: only hide direct children, not all descendants
sorenjohanson Jul 25, 2024
b8dfccf
changed: toolbar icon depending on node visibility, removed unnecessa…
sorenjohanson Jul 25, 2024
fc9fe67
changed: remove unnecessary UserNodeProperties attribute
sorenjohanson Jul 25, 2024
68141bc
fix: hide all descendants properly based on their parent status
sorenjohanson Jul 26, 2024
84a6c25
add: hidden eye icon when updating nodes
sorenjohanson Jul 26, 2024
ea2dd7d
add: disable hide node button if no node/root node is selected
sorenjohanson Jul 26, 2024
5d4e097
changed: rename methods
sorenjohanson Jul 29, 2024
ea706ff
removed: unnecessary class
sorenjohanson Jul 29, 2024
4f197de
fix: remove text like visibility_off and link from exported image
sorenjohanson Jul 30, 2024
9ccde3b
Merge branch 'main' into 105-hide-show-child-nodes-of-a-branch
sorenjohanson Aug 9, 2024
36b5319
add: comments to explain slightly more complicated branch visibility …
sorenjohanson Aug 9, 2024
4f2288c
fix: lint
sorenjohanson Aug 9, 2024
0d010d5
changed: history should not include visibility changes, fixed: toolba…
sorenjohanson Aug 9, 2024
04e5176
changed: visibility hidden instead of filtering out nodes, set proper…
sorenjohanson Aug 10, 2024
bef9113
fix: eye icon not rendering during undo/redo
sorenjohanson Aug 12, 2024
250a84c
added: method to recursively hide child nodes and retain proper hidde…
sorenjohanson Aug 13, 2024
587016f
fix: lint
sorenjohanson Aug 13, 2024
5beaf21
changed: small changes to variable names
sorenjohanson Aug 13, 2024
d0be27c
fix: link icon disappearing
sorenjohanson Aug 13, 2024
85454a7
fix: refactoring
sorenjohanson Aug 14, 2024
ee483a7
fix: refactor updateNodeHidden
sorenjohanson Aug 14, 2024
63f58ec
changed: add comments
sorenjohanson Aug 14, 2024
de316ce
Merge branch 'main' into 105-hide-show-child-nodes-of-a-branch
sorenjohanson Aug 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion teammapper-frontend/mmp/src/map/handlers/draw.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ export default class Draw {
* @param {Node} node
*/
private checkForHiddenChildren(node: Node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method does not only check, but actually hides or even removes Dom icons. That should be reflected in the name, as its otherwise confusing

if (this.map.nodes.nodeChildren(node.id)?.filter(x => x.hidden).length > 0) {
if (node.hasHiddenChildNodes) {
this.setHiddenChildrenIcon(node)
} else {
this.removeHiddenChildrenIcon(node)
Expand Down
24 changes: 24 additions & 0 deletions teammapper-frontend/mmp/src/map/handlers/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,29 @@ export default class History {
} else if (this.checkSnapshotStructure(snapshot)) {
const previousData = this.map.export.asJSON()

// Find all nodes where we've set hasHiddenChildNodes in the previous map
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe apply refactoring: extract method, as this is a lot of logic only dedicating to the hiding of nodes.

const nodesWithHiddenChildren = previousData.filter(node => node.hasHiddenChildNodes)

// This method will recursively hide all children of children until none are left
const hideChildNodes = (parentId: string) => snapshot.filter(node => node.parent === parentId).forEach(node => {
node.hidden = true
hideChildNodes(node.id)
})

snapshot.forEach(snapshotNode => {
const nodeWithHiddenChildren = nodesWithHiddenChildren.find(x => snapshotNode.id === x.id)

if (nodeWithHiddenChildren) {
snapshotNode.hasHiddenChildNodes = true

// We need to iterate through the snapshot instead of using this.map.nodes.nodeChildren() to see if we need to set hidden attributes as the latter will not have new nodes added yet
snapshot.filter(node => node.parent === snapshotNode.id).forEach(node => {
node.hidden = true
hideChildNodes(node.id)
})
}
})

this.redraw(snapshot)

this.map.zoom.center('position', 0)
Expand Down Expand Up @@ -137,6 +160,7 @@ export default class History {
locked: mergedProperty.locked,
detached: mergedProperty.detached,
hidden: mergedProperty.hidden,
hasHiddenChildNodes: mergedProperty.hasHiddenChildNodes,
isRoot: mergedProperty.isRoot
}

Expand Down
56 changes: 32 additions & 24 deletions teammapper-frontend/mmp/src/map/handlers/nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import MmpMap from '../map'
import * as d3 from 'd3'
import DOMPurify from 'dompurify'
import { v4 as uuidv4 } from 'uuid'
import {Event} from './events'
import { Event } from './events'
import Log from '../../utils/log'
import Utils from '../../utils/utils'

Expand Down Expand Up @@ -85,8 +85,8 @@ export default class Nodes {
*/
public addNode = (userProperties?: UserNodeProperties, notifyWithEvent: boolean = true, updateHistory: boolean = true, parentId?: string, overwriteId?: string): Node => {
const parentNode: Node = userProperties.detached ? null :
parentId ? this.getNode(parentId) : this.getSelectedNode()
parentId ? this.getNode(parentId) : this.getSelectedNode()

const properties: NodeProperties = Utils.mergeObjects(this.map.options.defaultNode, userProperties, true) as NodeProperties

properties.id = overwriteId || uuidv4()
Expand All @@ -110,7 +110,7 @@ export default class Nodes {
this.map.history.save()
}

if(notifyWithEvent) this.map.events.call(Event.nodeCreate, node.dom, this.getNodeProperties(node))
if (notifyWithEvent) this.map.events.call(Event.nodeCreate, node.dom, this.getNodeProperties(node))
return node
}

Expand Down Expand Up @@ -162,7 +162,7 @@ export default class Nodes {
* @param {string} color
* @returns {void}
*/
public highlightNodeWithColor = (id: string, color: string, notifyWithEvent: boolean = true): void => {
public highlightNodeWithColor = (id: string, color: string, notifyWithEvent: boolean = true): void => {
if (id !== undefined) {
if (typeof id !== 'string') {
Log.error('The node id must be a string', 'type')
Expand All @@ -175,7 +175,7 @@ export default class Nodes {
if (background.style.stroke !== color) {
background.style.stroke = DOMPurify.sanitize(color)

if(notifyWithEvent) this.map.events.call(Event.nodeUpdate, node.dom, this.getNodeProperties(node))
if (notifyWithEvent) this.map.events.call(Event.nodeUpdate, node.dom, this.getNodeProperties(node))
}
} else {
Log.error('The node id is not correct')
Expand Down Expand Up @@ -216,7 +216,7 @@ export default class Nodes {
public toggleBranchVisibility = () => {
if (this.selectedNode) {
const children = this.getChildren(this.selectedNode);

const descendants = this.getDescendants(this.selectedNode).filter(x => !children.includes(x));

/**
Expand All @@ -235,13 +235,20 @@ export default class Nodes {
if (x.parent.hidden && !x.hidden) {
this.updateNode('hidden', true, false, false, false, x.id)
}

if (!x.parent.hidden && x.hidden) {
this.updateNode('hidden', false, false, false, false, x.id)
}
})
}

// Lengthy but definitive check to see if we have any hidden nodes after toggling
if (this.map.nodes.nodeChildren(this.selectedNode.id)?.filter(x => x.hidden).length > 0) {
this.selectedNode.hasHiddenChildNodes = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't this be set directly, when the button is clicked (hide children)? Do we have to calculate this dynamically?

Copy link
Member

@JannikStreek JannikStreek Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see it, this property makes sense. So we have two new fields:

  • hidden (on the descendants)
  • hiddenBranches (on the parent)

So I would set them both upon button press?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a computed property before, but there's an issue with the history if we make it computed: Suppose that parent node A has exactly one child node B which is hidden. If you undo/redo the creation of the child node B, there's no way to tell it's supposed to be hidden, as the previous map (client side) doesn't include child node B, only the snapshot we receive from the server does (which doesn't have the client side "hidden" attribute). That's why we have hasHiddenChildNodes, which is set on parent node A.

So now we have two attributes:

  • hasHiddenChildNodes, which is always set on the parent node of any hidden children
  • hidden, which is set on the actual nodes that are hidden

The former only affects history and rendering of the hidden eye icon, the latter affects whether the node is actually visible or not. Both of them are set when calling toggleBranchVisibility().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, maybe you can add this reason as a comment? that there is not only the button that triggers it and thats the reason you have to check here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I've added some comments.

} else {
this.selectedNode.hasHiddenChildNodes = false
}

this.map.draw.update()
this.map.history.save()
}
Expand All @@ -251,7 +258,7 @@ export default class Nodes {
* Deselect the current selected node.
*/
public deselectNode = () => {
if(this.selectedNode?.id === this.getRoot().id) return
if (this.selectedNode?.id === this.getRoot().id) return

const oldNodeProps: ExportNodeProperties = this.getNodeProperties(this.selectedNode)
const oldDom: SVGGElement = this.selectedNode.dom
Expand Down Expand Up @@ -338,7 +345,7 @@ export default class Nodes {
if (graphic === false && updated !== false && updateHistory) {
this.map.history.save()
}

if (graphic === false && updated !== false && notifyWithEvent) {
this.map.events.call(Event.nodeUpdate, node.dom, { nodeProperties: this.getNodeProperties(node), changedProperty: property, previousValue })
}
Expand Down Expand Up @@ -371,7 +378,7 @@ export default class Nodes {

this.map.history.save()

if(notifyWithEvent) this.map.events.call(Event.nodeRemove, null, this.getNodeProperties(node))
if (notifyWithEvent) this.map.events.call(Event.nodeRemove, null, this.getNodeProperties(node))

this.deselectNode()
} else {
Expand Down Expand Up @@ -419,11 +426,12 @@ export default class Nodes {
image: Utils.cloneObject(node.image) as Image,
colors: Utils.cloneObject(node.colors) as Colors,
font: Utils.cloneObject(node.font) as Font,
link: Utils.cloneObject(node.link) as Link,
link: Utils.cloneObject(node.link) as Link,
locked: node.locked,
isRoot: node.isRoot,
detached: node.detached,
hidden: node.hidden,
hasHiddenChildNodes: node.hasHiddenChildNodes,
k: node.k
}
}
Expand Down Expand Up @@ -601,15 +609,15 @@ export default class Nodes {
* @param {string} id
* @returns {Node}
*/
public getNode = (id: string): Node => {
if (id !== undefined) {
if (typeof id !== 'string') {
Log.error('The node id must be a string', 'type')
return
}
return this.nodes.get(id)
public getNode = (id: string): Node => {
if (id !== undefined) {
if (typeof id !== 'string') {
Log.error('The node id must be a string', 'type')
return
}
return this.nodes.get(id)
}
}

/**
* Return the siblings of a node.
Expand Down Expand Up @@ -638,9 +646,9 @@ export default class Nodes {
*/
private calculateCoordinates(node: Node): Coordinates {
let coordinates: Coordinates = {
x: node.parent ? node.parent.coordinates.x : node.coordinates.x || 0,
y: node.parent ? node.parent.coordinates.y : node.coordinates.y || 0
},
x: node.parent ? node.parent.coordinates.x : node.coordinates.x || 0,
y: node.parent ? node.parent.coordinates.y : node.coordinates.y || 0
},
siblings: Array<Node> = this.getSiblings(node)

if (node.parent && node.parent.isRoot) {
Expand All @@ -658,7 +666,7 @@ export default class Nodes {
coordinates.x += 200
siblings = rightNodes
}
} else if(!node.detached) {
} else if (!node.detached) {
if (node.parent && this.getOrientation(node.parent)) {
coordinates.x -= 200
} else {
Expand All @@ -670,7 +678,7 @@ export default class Nodes {
const lowerNode = this.getLowerNode(siblings)
coordinates.y = lowerNode.coordinates.y + 60
} else if (!node.detached) {
coordinates.y -= 120
coordinates.y -= 120
}

return coordinates
Expand Down
2 changes: 2 additions & 0 deletions teammapper-frontend/mmp/src/map/models/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default class Node implements NodeProperties {
public isRoot: boolean
public detached: boolean
public hidden: boolean
public hasHiddenChildNodes: boolean

/**
* Initialize the node properties, the dimensions and the k coefficient.
Expand All @@ -39,6 +40,7 @@ export default class Node implements NodeProperties {
this.isRoot = properties.isRoot
this.detached = properties.detached
this.hidden = properties.hidden
this.hasHiddenChildNodes = properties.hasHiddenChildNodes

this.dimensions = {
width: 0,
Expand Down
33 changes: 18 additions & 15 deletions teammapper-frontend/mmp/src/map/options.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Colors, Coordinates, Font, Image, Link} from './models/node'
import { Colors, Coordinates, Font, Image, Link } from './models/node'
import Utils from '../utils/utils'
import Map from './map'
import * as d3 from 'd3'
Expand Down Expand Up @@ -136,17 +136,17 @@ export default class Options implements OptionParameters {
* Update edit behavior.
* @param {boolean} flag
*/
private updateEdit(flag: boolean) {
if (typeof flag !== 'boolean') {
Log.error('The value must be a boolean', 'type')
}

this.edit = flag

this.map.draw.clear()
this.map.draw.update()
private updateEdit(flag: boolean) {
if (typeof flag !== 'boolean') {
Log.error('The value must be a boolean', 'type')
}

this.edit = flag

this.map.draw.clear()
this.map.draw.update()
}

/**
* Update zoom behavior.
* @param {boolean} flag
Expand Down Expand Up @@ -188,8 +188,8 @@ export const DefaultNodeValues: DefaultNodeProperties = {
href: ''
},
coordinates: {
x: 0,
y: 0
x: 0,
y: 0
},
image: {
src: '',
Expand All @@ -208,6 +208,7 @@ export const DefaultNodeValues: DefaultNodeProperties = {
},
locked: true,
detached: false,
hidden: false,
isRoot: false,
}

Expand All @@ -217,8 +218,8 @@ export const DefaultRootNodeValues: DefaultNodeProperties = {
href: ''
},
coordinates: {
x: 0,
y: 0
x: 0,
y: 0
},
image: {
src: '',
Expand All @@ -237,7 +238,8 @@ export const DefaultRootNodeValues: DefaultNodeProperties = {
},
locked: true,
detached: false,
isRoot: true
isRoot: true,
hidden: false
}

export interface DefaultNodeProperties {
Expand All @@ -250,6 +252,7 @@ export interface DefaultNodeProperties {
locked: boolean
detached: boolean
isRoot: boolean
hidden: boolean
}

export interface OptionParameters {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class MapSyncService implements OnDestroy {
this.availableColors = COLORS;
this.clientColor =
this.availableColors[
Math.floor(Math.random() * this.availableColors.length)
Math.floor(Math.random() * this.availableColors.length)
];
this.modificationSecret = '';
this.colorMapping = {};
Expand Down
Loading