Closed stevemayhew closed 13 years ago
Hey Steve,
Thanks for your kind words and interest in Marco Polo! :)
I think I understand your changes, but can you explain your needs and motivations behind the changes a bit? Why do you need an external cache? Can you give me an example of how you'd use it specifically?
Also, I see a bug in my code on line 901 which you caught: results are cached even if the cache is disabled. I'll commit a fix for that soon.
Thanks!
Justin
Plans are to use the plugin in a couple of places. One for completing email addresses with manifest strait into a text box, other is to add a list of users / permissions, again using a text box to filer/search but the list of items in the manifest would be slightly different (username with permission value drop down in a list).
In both cases the list of users (quite large) is already locally stored in a JS object, so we will search and return values from it for the cache.fetch() callback, and largely ignore the cache.add() callback.
Seems to me the most flexible way to kill two birds with one bit of code: a) allow more sophisticated caching schemes, b) allow for sources other then an ajax request made by jquery.manifest.js
I'll check, but I'm pretty sure I can share the example on a Gist (permission widget will really show how flexible the manifest/marcopolo plugin is).
Let me flesh it out more (and test!), get the example put up, and get your feedback before you pull it. Sound good?
Hey Steve,
Yes, seeing an example would be very helpful.
I've received requests in the past to allow for a local source (rather than ajax), and I think it's a logical next step. At this point, however, allowing for a custom caching setup isn't something I see happening. I'm much more in favor of nailing the local source feature than killing two birds with one bit of code, as you say. I agree that it's more flexible, but I feel like the complexity it brings to simply using a local source outweighs the advantages.
Justin
Justin, Sounds great. I'm happy to take on the local source, as that would solve our needs as well. We sort of need both. Our use case is that the server will search for large user counts (eg 20k or so) and return a partial result (indicating this in the JSON), or for smaller numbers of user (5k or less), it effectively ignores the search ('q' parameter) and returns all the results. Looking at Marco Polo, considered using the formatData() to do the filtering, would work but need the q value passed (could get it from the $input).
For the cache to be useful however in this second case, we cannot store the unfiltered result keyed by the 'q' search string.
Also, FWIW, long running applications will want to limit the number of cached results, so I still think there is value in externalizing the add/fetch callbacks to others (and the code cost is small)
So, in short I agree with you. a 'dataSource' parameter or overloading/replacing the url is a good step. If you want to chat (Skype) or simply forward more requirements to me I'm happy to add the code.
So, I've got have the cache solution working for my use case. Externalizing the Ajax call is a nice to have for me (I agree, it's a more elegant solution, but understand it is on my time).
Here's the basic plan: refactor, extract method: the block of code in _request() that makes the Ajax call refactor, extract method: the anonymous function in the $.ajax success add an event 'requestCancelled' and signal it in the _cancelPendingRequest() method.
You can feel free to pick the names, maybe: requestData: function(q, self) returnedData:function(data) The error handling should be outside the scope of MarcoPolo in this use case, so I think no need to expose _buildErrorList. However you may want to expose show/hide and possibly empty the list.
The complete handler is also not required, as any busy indication would be dealt with outside of MarcoPolo.
Your thoughts?
-s
On Oct 14, 2011, at 6:49 AM, Justin Stayton wrote:
Hey Steve,
Yes, seeing an example would be very helpful.
I've received requests in the past to allow for a local source (rather than ajax), and I think it's a logical next step. At this point, however, allowing for a custom caching setup isn't something I see happening. I'm much more in favor of nailing the local source feature than killing two birds with one bit of code, as you say. I agree that it's more flexible, but I feel like the complexity it brings to simply using a local source outweighs the advantages.
Justin
Reply to this email directly or view it on GitHub: https://github.com/jstayton/jquery-marcopolo/pull/7#issuecomment-2406705
Hey Steve,
Let me summarize everything you're wanting to add, just so you can tell me if we're on the same page:
success
callback).requestCancelled
event when an ajax request is cancelled.Does that encompass everything? Sorry, I just want to make sure I'm understanding everything you're saying. :)
Justin
Justin, That looks good.
If you have a change we could Skype on these to make sure we are on the same page. Also, I can do small bits of things and commit them to my repository so you can 'cherry pick'. My goals of course are to minimize my merges when you add new features (and I suspect visa versa ;-))
I noticed you had added limited placeholder support. Might I suggest looking into simply supporting one of the existing ones: https://github.com/mathiasbynens/Placeholder-jQuery-Plugin is what we are using (I wish he would figure out how to make $(input).val() not return the placeholder text ever!). I played with this a bit with Manfiest, last trick is to never show the placeholder whenever there are values in the manifest.
Hey Steve,
Yes, I added overlabel/placeholder support for the case where minChars
is set to 0 (which causes a request when there's no input value, which I use to show all available options). I didn't want to build-in such support, but due to the way the input is focused and blurred, I had to to prevent the label from showing at unwanted times (like when an item is being selected with a mouse click, which causes the input to blur and the label to show). It was easiest to roll my own code for that (as opposed to integrating and keeping someone else's code up-to-date). If you're not working with minChars
set to 0, however, you should be able to use whatever overlabel/placeholder plugin you want.
After thinking more about your four feature requests, I'm inclined to say that the ability to search local data is the only one that I see being added. I haven't thought much about how I'd want to implement that yet, but you can certainly give it a shot in the meantime.
As I receive more feedback from other developers, I may consider the other three if there's enough of a need. Until then, please feel free to build those out in your fork and let me know when you want me to take a look. I will certainly cherry pick when I feel that a feature is compelling enough for the masses and fits in with my vision for the plugin. :)
Thanks again for your interest!
Justin
Yeah, I'll pull and have a look, thanks we needed the placeholder stuff. Marcopolo is a better place for it then manifest.
From Steve's iPhone
On Oct 16, 2011, at 7:38 PM, Justin Staytonreply@reply.github.com wrote:
Hey Steve,
Yes, I added overlabel/placeholder support for the case where
minChars
is set to 0 (which causes a request when there's no input value, which I use to show all available options). I didn't want to build-in such support, but due to the way the input is focused and blurred, I had to to prevent the label from showing at unwanted times (like when an item is being selected with a mouse click, which causes the input to blur and the label to show). It was easiest to roll my own code for that (as opposed to integrating and keeping someone else's code up-to-date). If you're not working withminChars
set to 0, however, you should be able to use whatever overlabel/placeholder plugin you want.After thinking more about your four feature requests, I'm inclined to say that the ability to search local data is the only one that I see being added. I haven't thought much about how I'd want to implement that yet, but you can certainly give it a shot in the meantime.
As I receive more feedback from other developers, I may consider the other three if there's enough of a need. Until then, please feel free to build those out in your fork and let me know when you want me to take a look. I will certainly cherry pick when I feel that a feature is compelling enough for the masses and fits in with my vision for the plugin. :)
Thanks again for your interest!
Justin
Reply to this email directly or view it on GitHub: https://github.com/jstayton/jquery-marcopolo/pull/7#issuecomment-2424426
Thanks for an excellent bit of code! Am planning to use this with manifest, one thing we needed to do to manage a rather large list of users locally in the browser was to keep and manage the cache, so I made this change:
"Expand the 'cache' options to allow using an external cache. This could also be used to have completely local results for some URL's, as well as implementing some more time based or memory sensitive cache functionality"
If you like the direction it's going or have suggestions, please let me know. I'm happy to flesh it the rest of the way out and update the API/docs when done.
Also, we're looking at a fix to enable HTML5 placeholder to work correctly as the overlabel.
Happy to contribute anyway I can.
Steve