Replace jstree with jquery ui sortable #569

Sinetheta · 2015-12-04T19:59:44Z

We were using jsTree in this one place only, to draw a taxonomy tree and let people drag around taxon leafs. Since we already have jQuery UI sortable, I thought that we could eliminate a dependency.

Changes

Removed jsTree.
Removed the rabl templates that it was using to fetch the taxonomy structure.
Used an existing api endpoint /api/taxonomies/2?set=nested to render the entire tree.
All levels of nesting are now always visible (are we worried about someone having 1000 taxons?).
Edit and Delete are now buttons on each taxon instead of a context menu.
Same-page rename is now gone. I would be happy to reimplement that if we can agree how it should work. But I don't know of another place in the backend where we offer it.
Rollback is gone. The new error functionality is "omg, fetch and redraw the tree".

Before

After

cbrunsdon · 2015-12-04T21:27:26Z

[insert pictures with explanation too, please. would be appreciated].

Sinetheta · 2015-12-04T22:48:25Z

@Mandily @tvdeyen I'd love input on this page while I'm editing. The biggest concerns for me are around adding new taxons.

On the old page you would right click a taxon and "Add" a new one as the new last child. That taxon would be called "New node" and your focus would be automatically set to edit that name in-page.

My current implementation has only an "Add Taxon" button in the header. Clicking that button inserts a taxon called "New node" as the new last child of the root taxon. In order to rename any new taxons a user would have to "edit" that taxon by leaving the page.

That was the easiest thing to implement, but I'd be happy to entertain alternative interactions.

jhawthorn · 2015-12-04T23:06:47Z

Thanks Kevin. jstree was terrible to use, flakey, and made us keep around a lot of cruft (custom json routes). I am very happy to see it go.

tvdeyen · 2015-12-04T23:07:19Z

Fun fact: I also have a branch for removing jstree, but hadn't a chance to work on it any further.

I use the jquery nested sortable plugin. A new dependency, yes, but a very small one, though.

The reason why I choosed this plugin was: nesting. Jquery sortable does not support nesting; only sorting elements on the same level. But we want to be able to move a node into another one, right?

Regarding the context menu: I would remove it and add buttons besides each node to add a child node.

We should also clean up the table columns. AwesomeNestedSet does not need a position column and nested sets in general work with parent, left and right columns. But the taxons table has all of them.

Maybe I can free some time to publish my branch. Would love to here your feedback on this.

BenMorganIO · 2015-12-04T23:09:37Z

api/config/routes.rb

-      end
-      resources :taxons do
-        member do
-          get :jstree


We should probably drop this into an extension in case anyone has an iOS or Android app relying on these endpoints.

Just saying, removing from the API might really upset anyone who wants to upgrade, but they're using this in their iOS or Android app. I know my previous employer Kabuni is using it in their iOS app.

I'm more a fan of killing it, as it wasn't ever actually published in the public API on the old docs site:
https://guides.spreecommerce.com/api/taxonomies.html

We can make a note in the release notes pointing to the commit if they want to pull that code into their own stores. I would consider that a better upgrade path than supporting it as an extension.

Though not removing the routes and just axing the javascript is still a good improvement... Maybe we do leave it till we just deprecate the entirety of the API.

@Sinetheta yea if you can at least pull those commits that remove the ruby side out into a separate PR it would be nice.

Sinetheta · 2015-12-04T23:59:25Z

Oh, but I'm sneaky @tvdeyen

$('#taxonomy_tree ul').sortable
  connectWith: '#taxonomy_tree ul'

Bam! nested sortables.

Pull it down. Play around a bit. I think it works pretty slick.

tvdeyen · 2015-12-05T00:20:32Z

Ha! You are great @Sinetheta
I don't have the time right now, to play with it. Will do tomorrow, but this is very neat!

Mandily · 2015-12-05T00:59:10Z

I'd like to see the original 'move' icon used instead of the 'sortable' icon you've used here. A bit more space between the two icons would be nice too, so we can minimize the chance of the user missing their target and clicking on the wrong icon.

Did you/can you remove that error message when the user tries to drag a row to the top level? It would be better if we could just not allow them to do that in the first place.

Otherwise looks good!

tvdeyen · 2015-12-05T22:02:17Z

I tested it and find it very hard to drag a node into another one. It's so hard to hit the trigger, that I thought
at first it is not even possible. I tried to use tolerance: "pointer", but this didn't help either.
And even if one finally manages to move a node into another one, it is nearly impossible to move it out again. Not a great UX IMO.

I am aware, that another dependency is not pleasant, but swapping out jstree with jquery.nestedSortable is still a total win of 148kb and a much better UX for the user.
Please try the demo at http://mjsarfatti.com/sandbox/nestedSortable/ to feel the difference.

If you still want to go only with jquery.ui.sortable I'm fine with that, but we should make the handling much smoother then, otherwise it does not feel like a good change UX wise.
Maybe some css (tried raising min-height for #taxonomy_tree ul) will do the trick.

Sinetheta · 2015-12-07T23:03:41Z

Thanks for taking a look at it @tvdeyen. Can you explain which drag operations were problematic. Personally I found it at least as functional as jsTree was (dragging any tree structure is going to require playing around until you get the hang of the particular implementation).

One thing that was giving me a problem, was trying to get something into the bottom most position of any list (it always wanted to be a child, not a sibling of the element above it), so I tweaked some settings and I think it feels more intuitive now.

I'd be happy to give nestedSortable a shot though if this still isn't feeling good enough.

tvdeyen · 2015-12-08T23:51:38Z

Feels much better now. Great work, Kevin!

There are still smaller UI glitches, but the dragging is perfect.

Great to see that we don't need any additional plugin for that feature.

Regarding the routes: Deprecate and then remove with the next release.

We're going to rebuild the taxonomy views with jquery UI

Sinetheta · 2015-12-09T19:35:31Z

I've added the api routes back in @BenMorganIO @cbrunsdon, we can just rip them out when we address the API as a whole.

I think we're good here, unless anyone has code related feedback, or UI/UX suggestions.

It can be tricky to drag a sortable to the final position of a list. Because of the placeholder empty lists we have placed there, it always wants to jump *into* the last item and become a child. `.ui-sortable-over` is applied to all of the ancestor lists of the placeholder to give a "lower edge" target (padding-bottom) and make it easier to sort to the last sibling position.

This guy is jsut kinda floating here, not doing much of anything. We can see that it's a tree.

jhawthorn · 2015-12-23T00:37:17Z

👍

cbrunsdon · 2015-12-24T17:53:19Z

Yea, I'm good on this one too, thanks @Sinetheta

athal7 · 2016-01-05T21:57:57Z

👍

Replace jstree with jquery ui sortable

This key was used by solidusio#569, but the translation key was missed.

mgharbik · 2016-01-27T16:46:44Z

I just noticed that when creating new taxons, they don't get the right parent_id, but taxonomy_id (it worked for me only for first taxonomies of sample data whose ids are the same).

I changed the js files that taxonomy_id is not really needed, otherwise I find it must work if we force to assign the parent_id in the api by removing the unless statement.

We are not using JSTree anymore (solidusio#569) but we still have these routes available for stores that used it outside JSTree scopes. We decided to deprecate these routes long time ago but we never did it. See also #194. Once we've deprecated this enough we can probably just resurrect solidusio#664.

Sinetheta force-pushed the remove-jstree branch from 86a767e to 7681138 Compare December 4, 2015 20:05

Sinetheta force-pushed the remove-jstree branch 2 times, most recently from 81b9475 to 822dd60 Compare December 4, 2015 22:47

BenMorganIO reviewed Dec 4, 2015
View reviewed changes

Sinetheta force-pushed the remove-jstree branch from 822dd60 to 51bcc2d Compare December 7, 2015 22:55

Remove jstree

083457d

We're going to rebuild the taxonomy views with jquery UI

Sinetheta force-pushed the remove-jstree branch from 51bcc2d to 0a093fb Compare December 9, 2015 18:33

Sinetheta added 11 commits December 9, 2015 11:50

Draw Taxonomy tree with Jquery UI sortable

d5fb1e5

Add dynamic sortable placeholder height

4c0cef1

Update taxon after sort

d2120fe

Add edit taxon link

9eae061

button_link_to should not modify options

91bb3f6

button_link_to should respect existing class option

db49d9a

Add create taxon button to edit taxonomy page

0a0e79c

Add delete taxon button

19d6234

Remove tree label

6f1060a

This guy is jsut kinda floating here, not doing much of anything. We can see that it's a tree.

Don't show sortable icon for root taxon

8a6ffc0

Sinetheta force-pushed the remove-jstree branch from 0a093fb to 8a6ffc0 Compare December 9, 2015 20:06

jhawthorn added this to the 1.2.0 milestone Dec 23, 2015

jhawthorn added a commit that referenced this pull request Jan 5, 2016

Merge pull request #569 from Sinetheta/remove-jstree

21038e6

Replace jstree with jquery ui sortable

jhawthorn merged commit 21038e6 into solidusio:master Jan 5, 2016

jhawthorn added a commit to jhawthorn/solidus that referenced this pull request Jan 6, 2016

Add missing add_taxon translation

bda2ce8

This key was used by solidusio#569, but the translation key was missed.

This was referenced Jan 6, 2016

Add missing add_taxon translation #660

Merged

Remove unused div #661

Merged

graygilmore mentioned this pull request Jan 7, 2016

Remove all remaining references to JSTree #664

Closed

mgharbik pushed a commit to mgharbik/solidus that referenced this pull request Jan 27, 2016

fixing the parent_id when creating a new taxon solidusio#569

a35ac1a

mgharbik mentioned this pull request Jan 29, 2016

Switching to closure tree #588 #662

Closed

kennyadsl mentioned this pull request Oct 4, 2017

Deprecate JSTree api routes #2254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace jstree with jquery ui sortable #569

Replace jstree with jquery ui sortable #569

Sinetheta commented Dec 4, 2015

cbrunsdon commented Dec 4, 2015

Sinetheta commented Dec 4, 2015

jhawthorn commented Dec 4, 2015

tvdeyen commented Dec 4, 2015

BenMorganIO Dec 4, 2015

Sinetheta Dec 5, 2015

BenMorganIO Dec 5, 2015

cbrunsdon Dec 8, 2015

cbrunsdon Dec 8, 2015

Sinetheta commented Dec 4, 2015

tvdeyen commented Dec 5, 2015

Mandily commented Dec 5, 2015

tvdeyen commented Dec 5, 2015

Sinetheta commented Dec 7, 2015

tvdeyen commented Dec 8, 2015

Sinetheta commented Dec 9, 2015

jhawthorn commented Dec 23, 2015

cbrunsdon commented Dec 24, 2015

athal7 commented Jan 5, 2016

mgharbik commented Jan 27, 2016

Replace jstree with jquery ui sortable #569

Replace jstree with jquery ui sortable #569

Conversation

Sinetheta commented Dec 4, 2015

Changes

Before

After

cbrunsdon commented Dec 4, 2015

Sinetheta commented Dec 4, 2015

jhawthorn commented Dec 4, 2015

tvdeyen commented Dec 4, 2015

BenMorganIO Dec 4, 2015

Choose a reason for hiding this comment

Sinetheta Dec 5, 2015

Choose a reason for hiding this comment

BenMorganIO Dec 5, 2015

Choose a reason for hiding this comment

cbrunsdon Dec 8, 2015

Choose a reason for hiding this comment

cbrunsdon Dec 8, 2015

Choose a reason for hiding this comment

Sinetheta commented Dec 4, 2015

tvdeyen commented Dec 5, 2015

Mandily commented Dec 5, 2015

tvdeyen commented Dec 5, 2015

Sinetheta commented Dec 7, 2015

tvdeyen commented Dec 8, 2015

Sinetheta commented Dec 9, 2015

jhawthorn commented Dec 23, 2015

cbrunsdon commented Dec 24, 2015

athal7 commented Jan 5, 2016

mgharbik commented Jan 27, 2016