symfony-cmf / tree-browser-bundle

Integrates javascript tree browser with PHPCR and PHPCR-ODM.
https://cmf.symfony.com
Other
22 stars 17 forks source link

dnd requires move function with promise #146

Closed ElectricMaxxx closed 7 years ago

ElectricMaxxx commented 7 years ago

With the current code we need an method on options: options.request.move() to do the move and return a kind of a promise. This option needs to be injected from the SonataPHPCRAdminBundle. I see two solutions:

  1. Just return an function () {return url;} as for load (our fancytree method loads alone) and handle the request on our side.
  2. add request.move with an ajax call, returning the promise

At least we must add add thednd: {}` option (enabled will be set automatic). What do you think?

ElectricMaxxx commented 7 years ago

Add the end the options for request are inconsistent at all

request: {
   load: function () {
       return: {url: '/path/to/api'};
  },
  move: function () {
     return $.ajax{};
  }
}

Either both return an an object containing url, then the naming is confusing as the method names seems to do something or both should realy do something, but fancytree itselfs is able to load by objects containing a simple url.

dbu commented 7 years ago

do i understand correctly that the decision is whether users of the tree bundle (like sonata admin) get a call for the move operation, or the tree bundle would do the moving itself? i think being consistent with loading would make sense. so the backend adapter that knows how to load a node should also know how to move, or the move operation needs to be disabled.

ElectricMaxxx commented 7 years ago

jep, the only difference for both methods is request.load is passed to fancytree internals,which does the load and fill its tree data. request.move is used in our code on an event after dragging.

dbu commented 7 years ago

not sure how reusable the move can be made. i guess the more we can solve generically inside the tree bundle and just let the consumer do the actual http requests to the backend to execute the move, the better. does that help?

ElectricMaxxx commented 7 years ago

Thinking about that a little bit longer, i would say the user should do the move call on its own. Doing so, we would keep the implementation of the tree and its Api outside of the treebrowser and makes it reusable for other tree usages. That said, i only have to fix it in SonataPhpcrAdmin only.

ElectricMaxxx commented 7 years ago

Got the side of SonataPhpcrAdminBundle working but found an other issue. The action to move on ResourceRestBundle returns an empty string instead of the new resource, but our treebrowser expects it :-(

So we need to fetch it newly or rewrite the response on ResourcesRestBundle

dbu commented 7 years ago

i feel like returning the new resource as a result of the move operation would be the expected behaviour. unless wouter disagrees, lets fix ResourceRestBundle (does it currently return a "204 no content" or a 200 with empty body? if it returns a 204 its kindof a bc break to change to 200 with body)

ElectricMaxxx commented 7 years ago

@dbu we have had the discussion when implementing the move api. There we decided to return no content 204.

Am 24.04.2017 um 09:39 schrieb David Buchmann:

i feel like returning the new resource as a result of the move operation would be the expected behaviour. unless wouter disagrees, lets fix ResourceRestBundle (does it currently return a "204 no content" or a 200 with empty body? if it returns a 204 its kindof a bc break to change to 200 with body)

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/symfony-cmf/tree-browser-bundle/issues/146#issuecomment-296559404, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxW6jUuJUohI4_CysjfXvX92xDws4e5ks5rzFHBgaJpZM4ND7fR.

dbu commented 7 years ago

ok, then we either have to adjust the resource in our client before we return to the tree, or fix the tree to not require the resource. it knows where it should have moved to, if the status is ok.

ElectricMaxxx commented 7 years ago

@dbu when you would have a look into: https://github.com/sonata-project/SonataDoctrinePhpcrAdminBundle/pull/435 you would see that i still have a solution. No change on the treebrowser itself - just a second call. What i miss is the position and that would be a change in possibly 3 places: treebrowser, Admin, Resource.

Am 24.04.2017 um 09:43 schrieb David Buchmann:

ok, then we either have to adjust the resource in our client before we return to the tree, or fix the tree to not require the resource. it knows where it should have moved to, if the status is ok.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/symfony-cmf/tree-browser-bundle/issues/146#issuecomment-296560431, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxW6svn__tvGPIsBp9wDc-d7sbkVHHgks5rzFKPgaJpZM4ND7fR.

ElectricMaxxx commented 7 years ago

Fixed through #147