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

Observable cannot properly report the position of objects added using the before option #6

Closed
sbrunot opened this issue Mar 14, 2014 · 9 comments

Comments

@sbrunot
Copy link

sbrunot commented Mar 14, 2014

It seems to me that the TRAC issue https://bugs.dojotoolkit.org/ticket/17642 opened for dojo/Store still applies to dstore Observable.

@kriszyp
Copy link
Contributor

kriszyp commented Mar 31, 2014

Just FYI, we do plan to address this issue in the next month.

@sbrunot
Copy link
Author

sbrunot commented Apr 1, 2014

Thanks @kriszyp

@mercmobily
Copy link

Kris, very humbling advice from a very humble developer...
I hacked Observable heavily in order to make this work (I have it working on my own code):

  • Implement before in Memory.js. Do it so that the elements are actually placed in the right spot in the array. I sent Dylan a version of Memory that does just that (although you will probably do it with half the code, and twice as fast) This is to prevent things like these: https://github.com/SitePen/dgrid/blob/master/test/data/base.js#L292 (if before is part of the store API, then it should be honoured by the most basic store, Memory).
  • Implement before in JsonRest.js. This is crucial! The way I did it, was by adding a header, X-position-before. This way, the server is at least given a chance to position things in the right spot. Note: at the moment, Dojo doesn't allow empty headers andbeforecan be an ID, ornull. The ideal solution is to treat an empty value inX-position-before` as null.
  • Change Observable so that it has access to before. I have a rewrite here -- although it's a rewrite of the old observable:

https://github.com/mercmobily/hotplate/blob/master/node_modules/hotDojoStores/client/JsonObservable.js

Zero optimisation, it's meant to show exactly what I am doing. The main difference is that it does NOT assume that the data is stored locally. This means that we need to tell it where the data is meant to be placed To make this happen, there are some necessary API changes.
The main change is in store.notify, that has a third parameter representing the method's options. In order to locate an item, JsonObservable.js will have access to options.before (where options is of the method itself). Using JsonObservable.js means that dgrid items will relocate automatically over JSON Rest stores just by enabling DnD -- it's all working already).

Just my 2c. I tried to simplify things as much as possible, I hope it helps...

@mercmobily
Copy link

The is one more use-case that I address in https://github.com/mercmobily/hotplate/blob/master/node_modules/hotDojoStores/client/JsonObservable.js and I think it's worthwhile covering it in this ticket while it's being worked on:

That is: a list of comments, with a comment form at the top. The list of comments is an OnDemandList, displaying query sorted by Date. When a new comment gets added, I would like to add it to the top of the list (most recent comments are added to the top)

That's why in JsonObservable I have a placeNew option for the query: I set it to first and know that any new comments will be added to the top. This is a way for me to tell Observable "Don't worry, place it at the top, and the order will stick" -- if that makes sense.

It's very possible that there is a better solution to this use case -- something that allows Observable to place an item at the beginning or the end without doing a full refresh.

(Note: using before and specifying the ID of the first item wouldn't work because 1) This is for a collection where items are ordered by date 2) You might want to do a put without reading the grid first to work out the ID of the first item

@mercmobily
Copy link

I just realised that the link to my previous comment is incorrect -- sorry about that.
I just created #22 to add before support to Memory and Rest.

@mercmobily
Copy link

Hi,

These (proposed) changes are now in the options-before branch:

master...options-before

One Notes dgrid needs to be changed accordingly so that it passes before when doing DnD. I have a pull request there all ready to go for dgrid. However, my PR also (necessarily) also adds code to emit extra events in order to make it possible, for programs, to show a spinning wheel while the DnD is actioned. In my code, there is also a strong case for running the put() only and only of the previous remove() was successful. Again, I would have loved to write three pull requests, but the modifications overlapped each other a lot.

I will go through the code, and test it on a real world application, tomorrow.

Thank you for doing it.

@mercmobily
Copy link

I shouldn't write comments late at night. dstore already passes before -- apologies, I got confused. My dstore PR only deals with emitting events and preventing permanent deletion if remove() fails.

@kriszyp
Copy link
Contributor

kriszyp commented Jul 16, 2014

Have you had a chance to test this? Can we close this?

@mercmobily
Copy link

This is now fully fixed.

@kriszyp kriszyp closed this as completed Jul 16, 2014
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

No branches or pull requests

3 participants