Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

Topic/tree view dnd 1821 #4204

Closed
wants to merge 1 commit into from
Closed

Topic/tree view dnd 1821 #4204

wants to merge 1 commit into from

Conversation

derkoe
Copy link
Contributor

@derkoe derkoe commented Jan 14, 2020

This PR shows a simple example implementation of the design spec in #1821

There are still some issues:

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • clarity.design website / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #1821

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@bdryanovski
Copy link
Contributor

Hey @derkoe, thanks for the contribution but .. could you fork from v2 branch or master depending on the target version - right now there 153 commits and it's hard to see what exactly you are fixing.

Thanks!

@derkoe
Copy link
Contributor Author

derkoe commented Jan 15, 2020

@bdryanovski actually there is only 1 commit in this PR - the others are only to catch the branch up with master. We did the PR against this branch because @gnomeontherun suggested that in #1821

@d-m-s Alright, so here is what we can do to get started.
I have created a feature branch for us to work on. https://github.com/vmware/clarity/tree/topic/tree-view-dnd-1821 We should use this to submit smaller PRs against so we can build it up.

@gnomeontherun
Copy link
Contributor

To help, I just pushed an update to the tree view branch that makes it aligned with master. @derkoe can you rebase your commit onto the updated branch so we get a single commit here? Thanks!

@gnomeontherun gnomeontherun self-requested a review January 16, 2020 18:42
 - add CSS for drag and drop to clr-tree
 - add a simple demo for drag and drop tree

Signed-off-by: Christian Köberl <christian.koeberl@gmail.com>
@@ -0,0 +1,13 @@
<h4>Drag and Drop (recursive)</h4>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @derkoe, thank you for submitting this PR. This looks like a possible workaround to apply DnD to enable reorderable nodes in a recursive tree.

Unfortunately, I don't think we could include this PR as a feature in Clarity. First of all, we cannot recommend this API as this is adding extra div elements to place clrDroppable. Also, looking at the code, it doesn't look like it followed our coding guidelines as specified in here. On top of that, as you mentioned yourself, this doesn't work reliably in some instances. To make sure its reliability and robustness, we need to write tests for it.

@Shijir
Copy link
Contributor

Shijir commented Jan 22, 2020

Recordable tree nodes is a prominent feature that would require a significant effort. As such, we will need continual communication and overseeing from our accessibility experts and UX designers on its implementation. Finding a scalable and intuitive API for it alone (ideally, that works for both declarative and recursive trees) is a challenge and needs research on it. As regards to this specific PR, I don't think the solution provided here is robust and scalable enough to cover most use cases of recordable tree nodes. We have to be careful about what we recommend as a feature or API because once they are in, it will be really hard to take them out unless we are willing to introduce more breaking changes to remove them.

@Shijir
Copy link
Contributor

Shijir commented Feb 19, 2020

Closing this PR for now based on the reasonings I mentioned above.

@Shijir Shijir closed this Feb 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants