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

Feat/tree view #1691

Merged
merged 11 commits into from
Jul 6, 2023
Merged

Feat/tree view #1691

merged 11 commits into from
Jul 6, 2023

Conversation

Mahmoud-zino
Copy link
Contributor

@Mahmoud-zino Mahmoud-zino commented Jun 23, 2023

Linked Issue

Closes #1668

Description

Basic implementation of the tree view.

Changsets

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)

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
andir Andreas Rammhold
@changeset-bot
Copy link

changeset-bot bot commented Jun 23, 2023

🦋 Changeset detected

Latest commit: 14840f8

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 Minor

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
Copy link

vercel bot commented Jun 23, 2023

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

Name Status Preview Updated (UTC)
skeleton-docs ✅ Ready (Inspect) Visit Preview Jul 5, 2023 9:34pm

@Mahmoud-zino Mahmoud-zino marked this pull request as ready for review June 23, 2023 16:04
@endigo9740
Copy link
Contributor

endigo9740 commented Jun 23, 2023

@Mahmoud-zino unfortunately I'm short on time, but here's a some quick notes based on my initial audit:

  • The standard name for this component is "Tree View". I'm fine with <TreeView> + <TreeItem> or <TreeViewItem>
  • The Disclosure element is perfect for this use case. Note that some folks may request animation for this, but it's not possible due to limitations with the elements themselves. We went through this with the Accordion and finally gave in and used non-semenatic elements in order to facilitate this. However, in the case of the Tree View, I think this is PERFECT as is. It helps differentiate it from the Accordion in fact.
  • Make sure our default padding and gap sizes are p-4 and gap-4 to be consistent with other components
  • The use of variant-soft is consistent with list styles, which is great, but you might want to wrap the featured demo in a card - it is a bit hard to see on the gradient background (reference)
  • I think most of your use of ARIA attributes is correct, but I'm not sure if aria-selected="false" is supposed to be hardcoded like that? Definitely follow what the ARIA APG guidelines says here. Review their demos too if you can! (link)
  • Try to avoid setting dynamic values inline in the reactive classes. This can make the code hard to read.
    So before:
$: classesBase = `${cBase} ${$$slots.children ? '' : noContentPadding} ${$$props.class ?? ''}`;

After:

$: classesPadding = $$slots.children ? '' : noContentPadding;
// Reactive Classes
$: classesBase = `${cBase} ${currentPadding} ${$$props.class ?? ''}`;

Questions

  1. I'm not sure I understand the use of noContentPadding - can you explain this?
  2. Can we separate the padding from the indentation? Maybe add a specific indent: CssClasses prop specifically for providing a uniform indent style? Margin might be most appropriate for this. Folks might want a more pronounced indentation and not want to have to run the padding offset math in their heads.

That's all I've got time for today, but I'll try to circle back next week. Thanks for all your effort this week!

@Mahmoud-zino
Copy link
Contributor Author

@endigo9740
I have applied your suggestions.


Regarding the fixed aria-selected="false", I was forced into it, because having treeitem role requires an aria-selected and since we don't have selection in the basic example I just fixed it to false.


I was not sure if we wanted to keep using noContentPadding, It was there because when a child doesn't have children, the caret will not be displayed and the position of the items will be off. However I changed it now to instead of hiding the carret from the dom (svelte-if) I am now using css-visiblity classes. I think this solution is a lot better but let me know if you have other ideas.

@endigo9740
Copy link
Contributor

endigo9740 commented Jul 5, 2023

@Mahmoud-zino I've implemented a number of minor changes and updates to this component. Overall I think it's working really well, but there were a few nitpick things I adjusted.

Additionally, my OCD was bothering me that the non-expandable rows had a large empty space when hovering each item. I tried removing the space, but then things looked lopsided. So I've introduced a second hyphen symbol that takes the place when a caret is not shown. Let me know what you think about this. Users can opt to adjust the opacity on this, even setting it to opacity-0 to make it hidden like your original version.

Screenshot 2023-07-05 at 4 32 24 PM

@Mahmoud-zino
Copy link
Contributor Author

@endigo9740 yeah It looks really great now and the users have control over displaying it or not, which is always helpful 👍.

@endigo9740 endigo9740 merged commit 1226d90 into skeletonlabs:dev Jul 6, 2023
@nicolas-albert
Copy link

I search a UI framework with a Tree View and I just try this PR HERE.
It's a nice component but a lot should be done to be a complete Tree View, with DnD support, (multi-)selection ...
Do you have plan to add more features?
Thx

@skeletonlabs skeletonlabs deleted a comment from nicolas-albert Jul 6, 2023
@endigo9740
Copy link
Contributor

endigo9740 commented Jul 6, 2023

@nicolas-albert Skeleton is open source which means folks like yourself can chime in and make feature requests or even contribute your own changes directly as long as you follow our contribution guidelines:
https://www.skeleton.dev/docs/contributing

Given this feature was merged 30 minutes before you posted we can't go back in time and update this pull request specifically. However, we still have time before the next release if you have minor feature suggestions. Larger features might need more consideration and likely wouldn't make it into the first release (just short of 2 weeks from now)

Per your specific requests above:

but a lot should be done to be a complete Tree View

Our features are not intended to compete with any dedicated library. Dedicated libraries typically have one or more people focused specifically on a single feature, while we're focusing on a whole library. Our components are typically going to be simpler in nature as we treat them as the "primitives" (read: building blocks) for building large features. If there's a dedicated library that offers a lot more features, you're always welcome to substitute that in place of any of Skeleton's features.

DnD support

Do you mean drag and drop? If so I'd need to see a relevant use case for this. It's not immediately clear to me how this would work or what problem it would solve. We introducing our Tree View to focus purely on navigation at this time.

(multi-)selection

Multi-selection of what exactly? In a tree view your drill down the tree of expandable options. You can toggle multiple options right now. Maybe an example would help here?

@nicolas-albert
Copy link

@endigo9740 contributing can be an option. I evaluating alive Svelte UI libraries with a default nice look, a set of components in order to make an IDE like app, including file management and other concepts based on a treeview.
I've done a pure Svelte/HTML basic treeview and I'm just searching if one already exists or if I have to continue to developing it, or maybe contribute to one.

Thank you for your answer!

@endigo9740
Copy link
Contributor

@nicolas-albert yeah unfortunately I'm not aware of any alternatives for Svelte off hand. Maybe check Flowbite or Tailwind? They provide HTML + Tailwind elements that can be integrated with Skeleton's theme system:

These would be for the visible UI, but then allow you full control over the logic and functionality as you see fit.

You could also clone our component and modify as desired locally in your project.

The goal for our component was a simple drill down navigation for a website - so if you're needing something very specific and purpose built, that might mean you would be better off rolling your own solution through a custom component.

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.

Feature request for TreeView Component
3 participants