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

Handle priority changes #19

Closed
anantn opened this Issue Apr 23, 2013 · 26 comments

Comments

Projects
None yet
6 participants
@anantn
Contributor

anantn commented Apr 23, 2013

From: http://stackoverflow.com/questions/16159576/im-convinced-that-ref-auth-is-bugged

"If you want to see the bugs in AngularFire, change priority of items. Then you'll see AngularFire unable to correctly move the items in the collection"

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Aug 7, 2013

Member

Also http://stackoverflow.com/questions/18008457/angularfire-priority-changes-not-working-inserting-duplicate-items-instead

Additionally, primitives (e.g. if the value is a string) get converted to objects/arrays if a priority exists on the records. See this fiddle: http://jsfiddle.net/katowulf/dFsqC/

Member

katowulf commented Aug 7, 2013

Also http://stackoverflow.com/questions/18008457/angularfire-priority-changes-not-working-inserting-duplicate-items-instead

Additionally, primitives (e.g. if the value is a string) get converted to objects/arrays if a priority exists on the records. See this fiddle: http://jsfiddle.net/katowulf/dFsqC/

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Aug 7, 2013

Member

Here's a test case that should work when priorities function: http://jsfiddle.net/katowulf/vVk6E/6/, Ideally it would work with strings as values too?

Member

katowulf commented Aug 7, 2013

Here's a test case that should work when priorities function: http://jsfiddle.net/katowulf/vVk6E/6/, Ideally it would work with strings as values too?

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Aug 7, 2013

Contributor

Should be fixed by d368161 (for angularFireCollection at-least?)

Contributor

anantn commented Aug 7, 2013

Should be fixed by d368161 (for angularFireCollection at-least?)

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Aug 7, 2013

Member

Hmm. Using the current angularFire.js on gh-pages, and this fiddle, it's not sorting records by priority inside ng-repeat.

Member

katowulf commented Aug 7, 2013

Hmm. Using the current angularFire.js on gh-pages, and this fiddle, it's not sorting records by priority inside ng-repeat.

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Aug 7, 2013

Contributor

The canonical angularFire.js is in the master branch, the one on gh-pages is there only because the example on angularjs.org is linking to it (and when angular/angularjs.org#64 is merged we can remove it).

Contributor

anantn commented Aug 7, 2013

The canonical angularFire.js is in the master branch, the one on gh-pages is there only because the example on angularjs.org is linking to it (and when angular/angularjs.org#64 is merged we can remove it).

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Aug 7, 2013

Member

I've updated to the latest build in master, still seeing oddness when priorities change: http://jsfiddle.net/katowulf/vVk6E/7/

Member

katowulf commented Aug 7, 2013

I've updated to the latest build in master, still seeing oddness when priorities change: http://jsfiddle.net/katowulf/vVk6E/7/

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Aug 7, 2013

Member

It does work if I use ng-repeat="widget in widgets | orderBy:widget.priority", is that the intended solution? Shouldn't the records in angularFireCollection exactly match the Firebase order? See the fiddle, which shows the raw data in sorted order, but the ng-repeat results are inconsistent.

Member

katowulf commented Aug 7, 2013

It does work if I use ng-repeat="widget in widgets | orderBy:widget.priority", is that the intended solution? Shouldn't the records in angularFireCollection exactly match the Firebase order? See the fiddle, which shows the raw data in sorted order, but the ng-repeat results are inconsistent.

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Aug 7, 2013

Contributor

I think it's because priorities are getting set to strings when modified via the text box, but priorities initially (and on reset()) are numbers.

Contributor

anantn commented Aug 7, 2013

I think it's because priorities are getting set to strings when modified via the text box, but priorities initially (and on reset()) are numbers.

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Aug 7, 2013

Member

I've fixed the integer/string issue (silly dev, code is for experts!). But it's still a bit wonky, and occasionally tossing out duplicates: http://jsfiddle.net/katowulf/vVk6E/

Member

katowulf commented Aug 7, 2013

I've fixed the integer/string issue (silly dev, code is for experts!). But it's still a bit wonky, and occasionally tossing out duplicates: http://jsfiddle.net/katowulf/vVk6E/

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Aug 30, 2013

Contributor

This version seems to work for me with the latest angularfire: http://jsfiddle.net/vVk6E/12/. Please re-open if you still see the issue!

Contributor

anantn commented Aug 30, 2013

This version seems to work for me with the latest angularfire: http://jsfiddle.net/vVk6E/12/. Please re-open if you still see the issue!

@anantn anantn closed this Aug 30, 2013

@bennlich

This comment has been minimized.

Show comment
Hide comment
@bennlich

bennlich Sep 25, 2013

Hmm. I'm still seeing issues with the fiddle. Changing the priority of "a" to 7 works, but changing it back to 1 doesn't. Also, orderBy:priority will only work for numerical priorities. Maybe the angularFire lib should include a custom orderByPriority filter?

bennlich commented Sep 25, 2013

Hmm. I'm still seeing issues with the fiddle. Changing the priority of "a" to 7 works, but changing it back to 1 doesn't. Also, orderBy:priority will only work for numerical priorities. Maybe the angularFire lib should include a custom orderByPriority filter?

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Oct 2, 2013

Contributor

Good call! Re-opening.

Contributor

anantn commented Oct 2, 2013

Good call! Re-opening.

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Nov 14, 2013

Contributor

Ok, this should be fixed in the "v2" branch, working example here: http://jsfiddle.net/6ahZj/1/

I'll leave this ticket open and close it when we've merged the 'v2' branch into master.

Contributor

anantn commented Nov 14, 2013

Ok, this should be fixed in the "v2" branch, working example here: http://jsfiddle.net/6ahZj/1/

I'll leave this ticket open and close it when we've merged the 'v2' branch into master.

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Nov 26, 2013

Member

Here's a working example of drag and drop sort using the new orderByPriority filter with Angular 1.2.0 and angularFire 0.5-rc2

http://plnkr.co/edit/Hq9ABcUYAJhXNWD3jyGs?p=preview

Member

katowulf commented Nov 26, 2013

Here's a working example of drag and drop sort using the new orderByPriority filter with Angular 1.2.0 and angularFire 0.5-rc2

http://plnkr.co/edit/Hq9ABcUYAJhXNWD3jyGs?p=preview

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Dec 12, 2013

Contributor

v2 has been merged into master and the example linked by @katowulf appears to work as expected!

Contributor

anantn commented Dec 12, 2013

v2 has been merged into master and the example linked by @katowulf appears to work as expected!

@anantn anantn closed this Dec 12, 2013

@marekhrabe

This comment has been minimized.

Show comment
Hide comment
@marekhrabe

marekhrabe Dec 26, 2013

Hi guys, actually latest example form @katowulf doesn't really work. It looks like it does, but there is an error in JavaScript console and priority-setting callback (sortableOptions.stop) never gets called.

Uncaught TypeError: Object #<Object> has no method 'splice' @ sortable.js:83

Is just the example implementation broken or is the issue somewhere in the library?

Isn't the problem that we are not sorting an array (which is typical for JavaScript), but rather an object (which is more convenient in Firebase world)?

marekhrabe commented Dec 26, 2013

Hi guys, actually latest example form @katowulf doesn't really work. It looks like it does, but there is an error in JavaScript console and priority-setting callback (sortableOptions.stop) never gets called.

Uncaught TypeError: Object #<Object> has no method 'splice' @ sortable.js:83

Is just the example implementation broken or is the issue somewhere in the library?

Isn't the problem that we are not sorting an array (which is typical for JavaScript), but rather an object (which is more convenient in Firebase world)?

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Dec 26, 2013

Member

It's probably this bug in the RC, fixed in the official release? #172

Member

katowulf commented Dec 26, 2013

It's probably this bug in the RC, fixed in the official release? #172

@marekhrabe

This comment has been minimized.

Show comment
Hide comment
@marekhrabe

marekhrabe Dec 26, 2013

If official release means http://cdn.firebase.com/libs/angularfire/0.5.0/angularfire.js then demo still doesn't work due to the same error. Edit: and the same with latest angularfire.min.js from master.

marekhrabe commented Dec 26, 2013

If official release means http://cdn.firebase.com/libs/angularfire/0.5.0/angularfire.js then demo still doesn't work due to the same error. Edit: and the same with latest angularfire.min.js from master.

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Dec 26, 2013

Member

I just tried both the plunkr and the fiddles and they work without issue.

Please start a new issue and include os/browser versions and a working code repro to help us troubleshoot.

Member

katowulf commented Dec 26, 2013

I just tried both the plunkr and the fiddles and they work without issue.

Please start a new issue and include os/browser versions and a working code repro to help us troubleshoot.

@marekhrabe

This comment has been minimized.

Show comment
Hide comment
@marekhrabe

marekhrabe Dec 26, 2013

I don't think it's a browser issue. I tried your plnkr on Mac OS X 10.9.1 in Chrome stable, Chrome Canary, Firefox and Safari with exact same results. Even tried to use stable version instead of rc1 which is in the source there, but without luck.

Drag&Drop seems to work, but it is not reflected in Firebase (priority is not changed).

Reproducing: load this demo, open it's firebase forge to see changes and try to drag and drop an item.

Expected result: no JavaScript error and item priority updated. It should be visible in Forge that items changed order.

Result: Firebase doesn't get updated and error in JavaScript console is raised: Uncaught TypeError: Object #<Object> has no method 'splice'. (Seems like closely related to #172)

marekhrabe commented Dec 26, 2013

I don't think it's a browser issue. I tried your plnkr on Mac OS X 10.9.1 in Chrome stable, Chrome Canary, Firefox and Safari with exact same results. Even tried to use stable version instead of rc1 which is in the source there, but without luck.

Drag&Drop seems to work, but it is not reflected in Firebase (priority is not changed).

Reproducing: load this demo, open it's firebase forge to see changes and try to drag and drop an item.

Expected result: no JavaScript error and item priority updated. It should be visible in Forge that items changed order.

Result: Firebase doesn't get updated and error in JavaScript console is raised: Uncaught TypeError: Object #<Object> has no method 'splice'. (Seems like closely related to #172)

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Dec 26, 2013

Member

Thanks for the repro!

Member

katowulf commented Dec 26, 2013

Thanks for the repro!

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Jan 3, 2014

Contributor

Reopening so we can investigate this.

Contributor

anantn commented Jan 3, 2014

Reopening so we can investigate this.

@anantn anantn reopened this Jan 3, 2014

@anantn

This comment has been minimized.

Show comment
Hide comment
@anantn

anantn Feb 12, 2014

Contributor

@marekhrabe The problem with the plunker is that it's treating $scope.todos as an array, which it is not. We need to figure how to better make angularFire work wit ui-sortable. Issue #200 might help with this.

Contributor

anantn commented Feb 12, 2014

@marekhrabe The problem with the plunker is that it's treating $scope.todos as an array, which it is not. We need to figure how to better make angularFire work wit ui-sortable. Issue #200 might help with this.

@anantn anantn closed this Feb 12, 2014

@gaiapunk

This comment has been minimized.

Show comment
Hide comment
@gaiapunk

gaiapunk Mar 31, 2014

@katowulf @anantn You guys are both awesome! I just wanted to check in and see if any progress been made on this particular issue? My business partner and I are attempting to use angularFire with ui-sortable but have found no clear best practice, any assistance is much appreciated.

gaiapunk commented Mar 31, 2014

@katowulf @anantn You guys are both awesome! I just wanted to check in and see if any progress been made on this particular issue? My business partner and I are attempting to use angularFire with ui-sortable but have found no clear best practice, any assistance is much appreciated.

@katowulf

This comment has been minimized.

Show comment
Hide comment
@katowulf

katowulf Apr 1, 2014

Member

@gaiapunk What would you propose be done? See issue 200 and add your thoughts.

Member

katowulf commented Apr 1, 2014

@gaiapunk What would you propose be done? See issue 200 and add your thoughts.

@sivaramsi

This comment has been minimized.

Show comment
Hide comment

sivaramsi commented Oct 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment