bugfix/tree-view-relational-updates #1910

Mahmoud-zino · 2023-08-20T13:21:06Z

Linked Issue

Closes #1885
Closes #1884

Needs to be done

we are now using export functions to handle part of the relations, but it is displayed in the props table. we need a way to hide it.

Description

fixed relational updates for both recursive and non-recursive modes.

This is a basic diagram of how the tree-view relations behave:

Extra changes

Forward all events triggered in recursive mode to tree-view.
deleted the toggleAll functions (to handle all possible setups selectAll, deselectAll will need extreme effort for such a tiny feature -_-)

Changsets

bugfix: Tree-view relational updates are now working as expected.

Checklist

Please read and apply all contribution requirements.

This PR targets the dev branch (NEVER master)
Documentation reflects all relevant changes
Branch is prefixed with: docs/, feat/, chore/, bugfix/
Ensure Svelte and Typescript linting is current - run pnpm check
Ensure Prettier linting is current - run pnpm format
All test cases are passing - run pnpm test
Includes a changeset (if relevant; see above)

changeset-bot · 2023-08-20T13:21:09Z

🦋 Changeset detected

Latest commit: a3ee7fa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@skeletonlabs/skeleton	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2023-08-20T13:21:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
skeleton-docs	✅ Ready (Inspect)	Visit Preview	Aug 22, 2023 4:14pm

Sarenor · 2023-08-20T13:43:54Z

Didn't break while I was testing it :)
Have you tried providing an @type comment to the exported functions?

/**
 * called when X happens
 * @type {() => A|B|C}
 */

Mahmoud-zino · 2023-08-20T14:29:43Z

Didn't break while I was testing it :) Have you tried providing an @type comment to the exported functions?
/**
 * called when X happens
 * @type {() => A|B|C}
 */

@type will only change the type, but I wanted to hide it entirely because users will not need this information. and it is displaying the function body in the value which is hella ugly 😄

Sarenor · 2023-08-20T16:02:13Z

Damn. I hoped that'd overwrite a bit more. Ah well... :|

ak4zh · 2023-08-21T14:23:23Z

I am trying to render a pre-checked parent but the parent remains unchecked.

let multipleDD: TreeViewNode[] = [
	{
		content: 'Books',
		lead: '<i class="fa-solid fa-book-skull"></i>',
		open: true,
		indeterminate: false,
		checked: true,
		children: [
			{ content: 'Clean Code', value: 'Clean Code', checked: true },
			{ content: 'The Clean Coder', value: 'The Clean Coder', checked: true },
			{ content: 'The Art of Unix Programming', value: 'The Art of Unix Programming', checked: true }
		],
		value: 'books'
	},
	{
		content: 'Movies',
		lead: '<i class="fa-solid fa-film"></i>',
		children: [
			{ content: 'The Flash', value: 'The Flash' },
			{ content: 'Guardians of the Galaxy', value: 'Guardians of the Galaxy' },
			{ content: 'Black Panther', value: 'Black Panther' }
		],
		value: 'movies'
	},
	{
		content: 'TV',
		lead: '<i class="fa-solid fa-tv"></i>',
		value: 'tv'
	}
];

endigo9740 · 2023-08-21T16:19:47Z

@Mahmoud-zino I've implemented a hard filter for those three export function props as requested. See how I implemented this in my commit in case you need to modify this in the future. It's not a perfect solution, but should work for now.

I'm able to replicate the error @ak4zh reported above, beyond that everything else looks good to me.

ak4zh · 2023-08-21T16:43:52Z

I've encountered another issue.
I added a log at https://github.com/Mahmoud-zino/skeleton/blob/411b3b660a7ae159d03119abcecf1a350a01b3f9/packages/skeleton/src/lib/components/TreeView/TreeViewDataDrivenItem.svelte#L72, and I can see an infinite log loop. It appears that the code is stuck in some sort of recursive loop, leading to continuous execution of reactivity.

Mahmoud-zino · 2023-08-21T16:50:55Z

@ak4zh can you please provide your setup, the tree-view is a very tricky component so I really appreciate you digging in it and finding these juicy bugs 😉
also the issue with the checked parent is now solved, it seems I have forgot to add the value to group instead of checking the node itself.

ak4zh · 2023-08-21T17:06:13Z

@Mahmoud-zino

I just checked, and the parent item check is now working correctly.
It turns out the second issue was a mistake. I put in a log, and I saw more than 10000 logs in the console. But this seems to only happen when I add the log to the skeleton code while my app is already using the skeleton setup on my computer.

I was using the pull request changes using this method, probably that was inappropriate:
import TreeView from '/Users/ak4zh/skeleton/packages/skeleton/src/lib/components/TreeView/TreeView.svelte'

I fixed it by putting in the log, closing my app, and then starting the server again. Now everything seems fine – the count only goes up as I add or remove items in the tree. So, when I test checking and unchecking things, it's all good.

Mahmoud-zino · 2023-08-21T17:09:23Z

@ak4zh on other thoughts this could be a performance issue, I have opted to the change event, this should still work the same for the component, just calling the function fewer times and only as needed.
I would really appreciate it, if you do another test after I commit and tell me if everything is still working as expected.

ak4zh · 2023-08-21T17:13:51Z

Additionally I notice you are using the spread operator to add the node.value. Can we use push instead?
I am dealing with a huge list of items in the TreeView, so I assume push would be better.

ak4zh · 2023-08-21T17:19:57Z

Verified the functionality works fine after 012464c.
Triggers only when something changes.

Mahmoud-zino · 2023-08-21T18:37:14Z

Additionally I notice you are using the spread operator to add the node.value. Can we use push instead? I am dealing with a huge list of items in the TreeView, so I assume push would be better.

This is used to trigger reactivity in svelte, so if we want to use .push instead we will still have to do

array.push(value);
array = array;

which I think will have the same effect on speed + the spread method is the recommended way of doing this from the svelte docs https://svelte.dev/tutorial/updating-arrays-and-objects

ak4zh · 2023-08-21T19:38:54Z

Got it. Thanks for the explanation.
Waiting for this to be merged so I can make use of it.
The new feature of my app heavily depends on the TreeView, with the new changes that dropped in v2 and the above bug fixes it is now super easy to implement really powerful TreeViews.

Before this I was generating the tree view myself and it was messy code.
Excited to push it to production as soon as this is merged.

ak4zh · 2023-08-21T19:47:35Z

@Mahmoud-zino On another thought I realised since the recent changes, we've moved away from using reactivity. As a result, I proceeded to test with array.push, and everything appears to be functioning accurately.

Just adding a note.

endigo9740 · 2023-08-21T20:02:50Z

FYI my goal will be to review this again tomorrow. If we're confident all is well I'll aim to merge then. This will make it part of the latest v2 RC release.

ak4zh · 2023-08-21T22:27:12Z

I recently noticed that the selectAll and deselectAll functions are being removed in this pull request. I've been brainstorming ways to streamline this, and I came up with a somewhat unconventional solution. What if we try this approach?

let refreshKey = String(Math.random());

export const actions = {
	updateAll(subNodes: TreeViewNode[], updateField: 'checked'|'open', status: boolean) {
		subNodes.forEach(n => {
			n[updateField] = status;
			if (updateField === 'checked') n.indeterminate = false;
			if (n.children && n.children.length) actions.updateAll(n.children, updateField, status)
		})
		refreshKey = String(Math.random());
	},

	selectAll() {
		actions.updateAll(nodes, 'checked', true)
	},

	deselectAll() {
		actions.updateAll(nodes, 'checked', false)
	},

	expandAll() {
		actions.updateAll(nodes, 'open', true)
	},

	collapseAll() {
		actions.updateAll(nodes, 'open', false)
	}
}

	{#key refreshKey}
		{#if nodes && nodes.length > 0}
			<TreeViewDataDrivenItem bind:nodes on:change on:click on:toggle on:keydown on:keyup />
		{:else}
			<slot />
		{/if}
	{/key}

// +page.svelte
<svelte:fragment slot="preview">
	<TreeView bind:actions={multiDDAction} bind:nodes={multipleDD} selection multiple />
</svelte:fragment>
<svelte:fragment slot="footer">
	<button class="btn variant-filled" on:click={() => multiDDAction.selectAll()}>Select All</button>
	<button class="btn variant-filled" on:click={() => multiDDAction.deselectAll()}>Unselect All</button>
	<button class="btn variant-filled" on:click={() => multiDDAction.expandAll()}>Expand All</button>
	<button class="btn variant-filled" on:click={() => multiDDAction.collapseAll()}>Collapse All</button>
</svelte:fragment>

ak4zh · 2023-08-21T22:57:55Z

Here's my repo with above suggested changes for quick test, everything seems to work correctly, not sure if this hacky approach is good enough for Chris:
https://github.com/ak4zh/skeleton/tree/bugfix/tree-view-relational-updates

Mahmoud-zino · 2023-08-22T06:17:56Z

@Mahmoud-zino On another thought I realised since the recent changes, we've moved away from using reactivity. As a result, I proceeded to test with array.push, and everything appears to be functioning accurately.

Just adding a note.

just tested both variants there is no difference whatsoever (also on performance).
but please note this is not because we are not using reactivity, it is because the code is executed in onMount, and reactivity is still triggered in TreeViewItem.
However, I would like to keep it as is just to keep the code clear and the style constant, if we notice any issue in performance we can change it any time.

Mahmoud-zino · 2023-08-22T06:24:35Z

I recently noticed that the selectAll and deselectAll functions are being removed in this pull request. I've been brainstorming ways to streamline this, and I came up with a somewhat unconventional solution. What if we try this approach?

let refreshKey = String(Math.random());

export const actions = {
	updateAll(subNodes: TreeViewNode[], updateField: 'checked'|'open', status: boolean) {
		subNodes.forEach(n => {
			n[updateField] = status;
			if (updateField === 'checked') n.indeterminate = false;
			if (n.children && n.children.length) actions.updateAll(n.children, updateField, status)
		})
		refreshKey = String(Math.random());
	},

	selectAll() {
		actions.updateAll(nodes, 'checked', true)
	},

	deselectAll() {
		actions.updateAll(nodes, 'checked', false)
	},

	expandAll() {
		actions.updateAll(nodes, 'open', true)
	},

	collapseAll() {
		actions.updateAll(nodes, 'open', false)
	}
}

	{#key refreshKey}
		{#if nodes && nodes.length > 0}
			<TreeViewDataDrivenItem bind:nodes on:change on:click on:toggle on:keydown on:keyup />
		{:else}
			<slot />
		{/if}
	{/key}

// +page.svelte
<svelte:fragment slot="preview">
	<TreeView bind:actions={multiDDAction} bind:nodes={multipleDD} selection multiple />
</svelte:fragment>
<svelte:fragment slot="footer">
	<button class="btn variant-filled" on:click={() => multiDDAction.selectAll()}>Select All</button>
	<button class="btn variant-filled" on:click={() => multiDDAction.deselectAll()}>Unselect All</button>
	<button class="btn variant-filled" on:click={() => multiDDAction.expandAll()}>Expand All</button>
	<button class="btn variant-filled" on:click={() => multiDDAction.collapseAll()}>Collapse All</button>
</svelte:fragment>

key will destroy the component and recreate it, I would avoid it when possible.
also and this is why I deleted this feature, we have to provide a solution for non-recursive mode also, which is not a trivial task since we don't have direct reference to children. so for the time being I would say we let the user handle this and maybe in the future once we have a better solution we can add it to the tree view.

AdrianGonz97

I've made a couple of changes that get rid of some of the type assertions that weren't really necessary, as well as replacing the group = [...group, node.value] in favor of mutating the array. Svelte's tutorial may mention it as a convenient way to trigger reactivity, but I have strong opinions against it 😄.

Besides that, everything seems to be working perfectly. Awesome work!

Mahmoud-zino · 2023-08-22T17:38:23Z

@AdrianGonz97

but I have strong opinions against it 😄.

I would be interested in hearing your opinions 😄, we have a lot of places where we are using the spread operator to trigger interactivity, so if there is a problem other than the style it might be worth it to change them.

AdrianGonz97 · 2023-08-22T18:02:41Z

@Mahmoud-zino
The spread operator is horrendously slow. Like, several orders of magnitude slower than it's mutating counterpart. Mainly because it's allocating new memory every single time we use it. It certainly has its place for when you want to clone the values of an array into another, but when it comes to using it as a means to trigger reactivity, such as appending a new value to the array, it's horrific!

I like to reference this example when talking about the consequences of this approach:

And it's not like this is a large dataset either - it's only 1K elements!

As library authors, we should be, generally, cautious when it comes to performance, striving to be as performant as reasonably possible. Using arr.push(value) instead of arr = [...arr, value] would be one of the small steps towards that goal.

This method of reassignment is one of those React idioms that has weaseled it's way into Svelte. It's popular among the React folks because of how their reactivity system works. Over there, state is based on referential equality. Meaning, they have create a whole new array in order for the VDOM to recognized that their state has updated. Over here in Svelte land, we have the luxury of simply mutating the state and being done with it. No excess memory allocation required. We might as well take advantage of it!

/end-of-rant

Mahmoud-zino · 2023-08-22T18:12:17Z

oof, I though the different was trivial...
@AdrianGonz97 Thank you very much for this valuable infos, I will keep that in mind for the future 👍

fixed relational updates

501b5a2

vercel bot temporarily deployed to Preview August 20, 2023 13:22 Inactive

Added SveldMapper filter for Tree View props

411b3b6

vercel bot temporarily deployed to Preview August 21, 2023 16:20 Inactive

onMount add group when all children are checked

bdbf4db

vercel bot temporarily deployed to Preview August 21, 2023 16:43 Inactive

changed group change check to event instead of reactivity

012464c

vercel bot temporarily deployed to Preview August 21, 2023 17:11 Inactive

endigo9740 requested a review from AdrianGonz97 August 21, 2023 20:06

AdrianGonz97 added 3 commits August 22, 2023 11:57

adjusted types to replace type assertions

be92ed3

mutate the array rather than creating a new one

76f823d

replaced type assertions

a3ee7fa

vercel bot temporarily deployed to Preview August 22, 2023 16:14 Inactive

AdrianGonz97 approved these changes Aug 22, 2023

View reviewed changes

Mahmoud-zino mentioned this pull request Aug 22, 2023

Data-Driven Tree View Enhancements #1884

Closed

endigo9740 mentioned this pull request Aug 23, 2023

TreeView selection of item #1926

Closed

endigo9740 merged commit 82ec4f2 into skeletonlabs:v2 Aug 23, 2023

This was referenced Aug 23, 2023

chore(release): version package (rc) #1932

Merged

chore(release): version package #1958

Closed

chore(release): version package #1963

Merged

Uh oh!

Uh oh!

bugfix/tree-view-relational-updates #1910

bugfix/tree-view-relational-updates #1910

Uh oh!

Conversation

Mahmoud-zino commented Aug 20, 2023 • edited by endigo9740 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked Issue

Needs to be done

Description

Extra changes

Changsets

Checklist

Uh oh!

changeset-bot bot commented Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

vercel bot commented Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sarenor commented Aug 20, 2023

Uh oh!

Mahmoud-zino commented Aug 20, 2023

Uh oh!

Sarenor commented Aug 20, 2023

Uh oh!

ak4zh commented Aug 21, 2023

Uh oh!

endigo9740 commented Aug 21, 2023

Uh oh!

ak4zh commented Aug 21, 2023

Uh oh!

Mahmoud-zino commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ak4zh commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mahmoud-zino commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ak4zh commented Aug 21, 2023

Uh oh!

ak4zh commented Aug 21, 2023

Uh oh!

Mahmoud-zino commented Aug 21, 2023

Uh oh!

ak4zh commented Aug 21, 2023

Uh oh!

ak4zh commented Aug 21, 2023

Uh oh!

endigo9740 commented Aug 21, 2023

Uh oh!

ak4zh commented Aug 21, 2023

Uh oh!

ak4zh commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mahmoud-zino commented Aug 22, 2023

Uh oh!

Mahmoud-zino commented Aug 22, 2023

Uh oh!

AdrianGonz97 left a comment

Choose a reason for hiding this comment

Uh oh!

Mahmoud-zino commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdrianGonz97 commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mahmoud-zino commented Aug 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Mahmoud-zino commented Aug 20, 2023 •

edited by endigo9740

Loading

changeset-bot bot commented Aug 20, 2023 •

edited

Loading

vercel bot commented Aug 20, 2023 •

edited

Loading

Mahmoud-zino commented Aug 21, 2023 •

edited

Loading

ak4zh commented Aug 21, 2023 •

edited

Loading

Mahmoud-zino commented Aug 21, 2023 •

edited

Loading

ak4zh commented Aug 21, 2023 •

edited

Loading

Mahmoud-zino commented Aug 22, 2023 •

edited

Loading

AdrianGonz97 commented Aug 22, 2023 •

edited

Loading