-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Unable to send along extra options #6
Comments
I had this issue today too. So I've forked and created a pull request. I handled this by adding a 'params' option which gets merged into the store.find request. Pull request: #7 |
@joshudev Nice solution. However, wouldn't it be better to not use a params object and just extend the object itself? |
I did think about doing it that way, but then wondered whether it's cleaner to use a params object in case in future there will be options that need to be set that you wouldn't want getting passed on to store.find. I don't mind so much either way, but it's more explicit with the params object. |
Haha I agree on that as well 😄 . Both has it's pros and cons. Let's see what @hhff has to say about it |
Yeah I'm just happy I got me an infinite scroll! It's funny that I went to log the same issue only 30 mins after you did! |
Hey duders! Thanks so much for the PR! This is definitely something we need. Ultimately, all of the params will be passed into the I think we just need to jimmy-jangle the code so that it passes through the full object to the Thoughts? |
That sounds good to me! Also I noticed the travis build is failing on my fork - I think I should have used Ember.$.extend instead of $.extend.. anyways.. so will you add this feature quite soon? I'm using my fork on the project I'm working on for the moment. |
p.s If you want me to adjust the fork and test, I am happy to do so. |
I'd love if you updated your PR @joshudev ! A test or two would be nice also. Doesn't look like we'll need $.extend with this approach at all |
Nice, this all sounds like music to my ears, haha. Thank you guys! |
Came to post the same issue. Glad to see it's already underway! 👍 😄 |
Since it's been a few days, I've submitted a PR (#9) along the same lines with passing acceptance and unit tests. Comments welcome. |
Woo! reviewing this now |
closed via #9 |
For my application I would like to send along extra options, like sorting / active / etc. filters. Now I implemented some extra logic for me in the route.js, but I guess creating an issue would be the right thing to do :).
The text was updated successfully, but these errors were encountered: