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

TreeList Example #107

Closed
wants to merge 4 commits into from
Closed

TreeList Example #107

wants to merge 4 commits into from

Conversation

jpoles1
Copy link
Contributor

@jpoles1 jpoles1 commented Nov 3, 2023

Adding example for TreeList as well as some comments to the source file to help clarify functionality

Copy link

vercel bot commented Nov 3, 2023

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

Name Status Preview Comments Updated (UTC)
svelte-ux ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 3, 2023 6:26am

Copy link

what-the-diff bot commented Nov 3, 2023

PR Summary

  • Enhanced Readability and Usability of Node Structure in TreeList
    Our team added type definitions for the 'Node' component within the TreeList file. This change allows developers and users to better comprehend the structure and layout of the nodes within our application, enhancing overall usability.

  • Introduced Classes Prop in TreeList
    We've included an optional 'classes' parameter in the TreeList. This permits conditional classes to be applied on tags, offering a higher degree of customization and control over the look and feel of your elements.

  • Addition of Icon Component, Importing mdiCircleSmall
    In the page file, our developers imported the 'Icon' component and 'mdiCircleSmall' from the '@mdi/js' library. This introduces new, visually appealing icons into the interface, enriching user experience within the application.

  • Nodes Array Initiated with Sample Data
    The nodes array in the page file has been initialized with sample data. This provides a default demonstration of the nodes' presentation and data for the user/developer, enhancing understanding of our application's information structure.

  • New Section To Exhibit TreeList Component's Usage
    A new section was added in the page file to present a use case of the TreeList component with the provided sample data. This serves as a live example of the TreeList component's functionality and aids in illustrating its usage and capabilities to users and developers.

Copy link
Owner

@techniq techniq left a comment

Choose a reason for hiding this comment

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

Thanks @jpoles1! I left a few suggestions. I love that you are adding jsdoc comments as we need to do this for all components (which will take a bit).

type Node = { id: number; name: string; level: number; children: Node[] };

/**
* An array of nodes containing custom metadata and an array of child nodes
* @param {Array} nodes - An array of Node Objects each containing a children: Node[] attribute.
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be @type instead since it's not a function parameter (I'm not a jsdoc expert though).

/** @type {{ name: string; children: Node[] }[]} */

Doing so improves the docs (at least how Sveld parses them). See the orange unknown

Before After
image image

Copy link
Owner

Choose a reason for hiding this comment

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

I ment to link to the jsdoc @type docs - https://jsdoc.app/tags-type.html

Also, if we needed to do per-property comments, I think we would need to use this syntax...

/**
 * @typedef PropertiesHash
 * @type {object}
 * @property {string} id - an ID.
 * @property {string} name - your name.
 * @property {number} age - your age.
 */

/** @type {PropertiesHash} */
var props;

but I'm not sure it's worth it over just...

/** @type {{ id: string; name: string; age: number }} */

as I don't think it's handled by Sveld (or at least we have a way to show it in the docs). It could improve VSCode developer experience though (but haven't tested).

Thanks again for tackling this.

export let nodes: Node[];

/**
* Allows for conditional classes to be applied to each tag <ul> and <li>
* @param {Object} classes - An object containing optional classes for <ul> and <li> tags.
Copy link
Owner

Choose a reason for hiding this comment

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

Similarily, should this be...

/**
 * @type {{ul?: string|Function, li?: string|Function}}
**/

It's what I've used for Button, at least as a test to improve how Sveld parses them since it doesn't appear to read the typescript types in this case :(.

See also this comment in LayerChart (which applies to Svelte UX) as well with improving how classes are shown in the docs.

techniq/layerchart#39 (comment)

Basically what you're doing here, we need to do everywhere (unless we can find a way to have Sveld handle this, or another "automated" way to parse the typescript defs.

I also wrote up a little bit in the discord

Comment on lines +58 to +63
<Preview language="typescript" code="{"type Node = { name: string; level: number; children: Node[] };\n\nconst nodes =" + JSON.stringify(nodes, null, 2)}">
<pre>
type Node = &lbrace; name: string; level: number; children: Node[] &rbrace;;
const nodes = {JSON.stringify(nodes, null, 2).slice(0, 300)}...
</pre>
</Preview>
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if it might be nicer to add "svelte-json-tree as a devDependency like I did for LayerChart recently.

CleanShot.2023-11-03.at.10.01.03.mp4

I would think we would show it inline instead of via a dialog, but just a nice JSON preview. What do you think?

@techniq
Copy link
Owner

techniq commented Jan 4, 2024

Hey @jpoles1 I'm going to close this for now, but feel free to re-open and rebase off main once #167 is merged. Also see my comments regarding jsdoc.

@techniq techniq closed this Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants