search

amitava82 / angular-multiselect

[NOT MAINTAINED]Native AngularJS multiselect directive
http://amitava82.github.io/angular-multiselect
MIT License
140 stars 124 forks source link

Performance improvement #78

Open arindrajitb opened 8 years ago

arindrajitb commented 8 years ago

Code merged from https://github.com/zachlysobey/angular-multiselect and https://github.com/amitava82/angular-multiselect . new functionality added 1) added tracked by id or $index. id can be specified as attribute. Css can be applied to while list as well as list item. example updated to show both scenario. example updated to show object selection.

zachlysobey commented 8 years ago

Great to see some serious work being done here.

However, these are very difficult commits to review:

I can't speak for @amitava82, but I think it might be better if we re-do these changes as nice small commits, so as not to muck up the commit history? I'd be happy to volunteer some of my time, if you can help me with it?

Please correct me if I'm wrong, but in order of importance, this PR includes:

I think we should maybe focus on getting this stuff merged in piece by piece.

arindrajitb commented 8 years ago

To be frank when i saw the chekin history i was bit gutted. All line shows different because of reformatting. Too used to formatting files using keyboard shortcuts :(

Well changes are not a lot. I'll try to decipher it.

  1. I took your code , then merged it to Amitava82's trunk. That's my first commit. Dont know what changed there, I've not touched anything manually, all done by git merge.
  2. Introduced 'track by' in ng-repeat.
    • Index.html - line 42 introduced id filed to be used in tracking ' ms-id="id"
    • index.html - line 12 to 39, just visible doc. it includes the directive code. so its visible on the page and people wont have to do a view source.
    • changed car demo to select object as a whole options="c as c.name for c in cars"
    • multiselect.js line 51 - getting id into scope variable.
    • multiselect.js line 128 to 136: get id into items list. conditional if id is not there, nothing is added.
    • both multiselect and single select html file UL element block duplicated with ng-if. In ng-repeat added tracked by.
    • introduced style sheet: At this point it width is unlimitted. wanted to have option to reduce it. car example uses this option, where as fruit doesnot.
    • App.js added three extra item in both list line 8, 9, 16.
    • app.css added to with two class one for width another for word wrapping
    • index.html line 42 - list-css="fixed-width" list-item-css= "word-wrapped"
    • multiselect.js line 52 to 65- get the attributes in scope, added methods to decide css class.
    • css applied in multiselect html file in line 6,15, 19, 28
    • similarly in single select html line 6, 8, 12, 14
    • index.xml - just to get it working - updated angular and bootstrap .
    • added app.css
    • replaced multiselect.js and html with multiselect-tpls.js

by the way i had one extra commit, as bower_component was not in gitignore as a result library files got added. Updated the gitignore file. I removed them in last commit.

I think thats all :)

regards Ari

On 3 January 2016 at 21:24, Zach Lysobey notifications@github.com wrote:

Great to see some serious work being done here.

However, these are very difficult commits to review:

  • They touch many files that are not relevant to these feature updates.
  • Weird merging stuff is going on...
  • The feature changes in the src files are obfuscated by indentation changes.
  • Other refactoring (and I'm sure good improvements) is mixed in (esp on the demo)

I can't speak for @amitava82 https://github.com/amitava82, but I think it might be better if we re-do these changes as nice small commits, so as not to muck up the commit history? I'd be happy to volunteer some of my time, if you can help me with it?

Please correct me if I'm wrong, but in order of importance, this PR includes:

  • new performance features
  • updates to demo and documentation
  • demo improvements/refactorings (update angular version, bootstrap CDN, etc...)
  • code style changes
  • (anything I missed?)

I think we should maybe focus on getting this stuff merged in piece by piece.

— Reply to this email directly or view it on GitHub https://github.com/amitava82/angular-multiselect/pull/78#issuecomment-168541719 .

arindrajitb commented 8 years ago

This is how the doc page looks:

[image: Inline images 1]

[image: Inline images 2] [image: Inline images 3]

On 3 January 2016 at 22:02, Ari Biswas arindrajitb@gmail.com wrote:

To be frank when i saw the chekin history i was bit gutted. All line shows different because of reformatting. Too used to formatting files using keyboard shortcuts :(

Well changes are not a lot. I'll try to decipher it.

  1. I took your code , then merged it to Amitava82's trunk. That's my first commit. Dont know what changed there, I've not touched anything manually, all done by git merge.
  2. Introduced 'track by' in ng-repeat.
    • Index.html - line 42 introduced id filed to be used in tracking ' ms-id="id"
    • index.html - line 12 to 39, just visible doc. it includes the directive code. so its visible on the page and people wont have to do a view source.
    • changed car demo to select object as a whole options="c as c.name for c in cars"
    • multiselect.js line 51 - getting id into scope variable.
    • multiselect.js line 128 to 136: get id into items list. conditional if id is not there, nothing is added.
    • both multiselect and single select html file UL element block duplicated with ng-if. In ng-repeat added tracked by.
    • introduced style sheet: At this point it width is unlimitted. wanted to have option to reduce it. car example uses this option, where as fruit doesnot.
    • App.js added three extra item in both list line 8, 9, 16.
    • app.css added to with two class one for width another for word wrapping
    • index.html line 42 - list-css="fixed-width" list-item-css= "word-wrapped"
    • multiselect.js line 52 to 65- get the attributes in scope, added methods to decide css class.
    • css applied in multiselect html file in line 6,15, 19, 28
    • similarly in single select html line 6, 8, 12, 14
    • index.xml - just to get it working - updated angular and bootstrap .
    • added app.css
    • replaced multiselect.js and html with multiselect-tpls.js

by the way i had one extra commit, as bower_component was not in gitignore as a result library files got added. Updated the gitignore file. I removed them in last commit.

I think thats all :)

regards Ari

On 3 January 2016 at 21:24, Zach Lysobey notifications@github.com wrote:

Great to see some serious work being done here.

However, these are very difficult commits to review:

  • They touch many files that are not relevant to these feature updates.
  • Weird merging stuff is going on...
  • The feature changes in the src files are obfuscated by indentation changes.
  • Other refactoring (and I'm sure good improvements) is mixed in (esp on the demo)

I can't speak for @amitava82 https://github.com/amitava82, but I think it might be better if we re-do these changes as nice small commits, so as not to muck up the commit history? I'd be happy to volunteer some of my time, if you can help me with it?

Please correct me if I'm wrong, but in order of importance, this PR includes:

  • new performance features
  • updates to demo and documentation
  • demo improvements/refactorings (update angular version, bootstrap CDN, etc...)
  • code style changes
  • (anything I missed?)

I think we should maybe focus on getting this stuff merged in piece by piece.

— Reply to this email directly or view it on GitHub https://github.com/amitava82/angular-multiselect/pull/78#issuecomment-168541719 .

arindrajitb commented 8 years ago

by the way bower installation is not working. Its not installing dist folder. Need to include at least the following. can include all files in distribution as well.

"main": [ "./dist/multiselect-tpls.js", "./dist/multiselect.css" ],

and some trouble int the package names too. can't remember anymore. I've deleted that. ok, too many email. no more. njoy

On 3 January 2016 at 22:07, Ari Biswas arindrajitb@gmail.com wrote:

This is how the doc page looks:

[image: Inline images 1]

[image: Inline images 2] [image: Inline images 3]

On 3 January 2016 at 22:02, Ari Biswas arindrajitb@gmail.com wrote:

To be frank when i saw the chekin history i was bit gutted. All line shows different because of reformatting. Too used to formatting files using keyboard shortcuts :(

Well changes are not a lot. I'll try to decipher it.

  1. I took your code , then merged it to Amitava82's trunk. That's my first commit. Dont know what changed there, I've not touched anything manually, all done by git merge.
  2. Introduced 'track by' in ng-repeat.
    • Index.html - line 42 introduced id filed to be used in tracking 'ms-id="id"
    • index.html - line 12 to 39, just visible doc. it includes the directive code. so its visible on the page and people wont have to do a view source.
    • changed car demo to select object as a whole options="c as c.name for c in cars"
    • multiselect.js line 51 - getting id into scope variable.
    • multiselect.js line 128 to 136: get id into items list. conditional if id is not there, nothing is added.
    • both multiselect and single select html file UL element block duplicated with ng-if. In ng-repeat added tracked by.
    • introduced style sheet: At this point it width is unlimitted. wanted to have option to reduce it. car example uses this option, where as fruit doesnot.
    • App.js added three extra item in both list line 8, 9, 16.
    • app.css added to with two class one for width another for word wrapping
    • index.html line 42 - list-css="fixed-width" list-item-css= "word-wrapped"
    • multiselect.js line 52 to 65- get the attributes in scope, added methods to decide css class.
    • css applied in multiselect html file in line 6,15, 19, 28
    • similarly in single select html line 6, 8, 12, 14
    • index.xml - just to get it working - updated angular and bootstrap .
    • added app.css
    • replaced multiselect.js and html with multiselect-tpls.js

by the way i had one extra commit, as bower_component was not in gitignore as a result library files got added. Updated the gitignore file. I removed them in last commit.

I think thats all :)

regards Ari

On 3 January 2016 at 21:24, Zach Lysobey notifications@github.com wrote:

Great to see some serious work being done here.

However, these are very difficult commits to review:

  • They touch many files that are not relevant to these feature updates.
  • Weird merging stuff is going on...
  • The feature changes in the src files are obfuscated by indentation changes.
  • Other refactoring (and I'm sure good improvements) is mixed in (esp on the demo)

I can't speak for @amitava82 https://github.com/amitava82, but I think it might be better if we re-do these changes as nice small commits, so as not to muck up the commit history? I'd be happy to volunteer some of my time, if you can help me with it?

Please correct me if I'm wrong, but in order of importance, this PR includes:

  • new performance features
  • updates to demo and documentation
  • demo improvements/refactorings (update angular version, bootstrap CDN, etc...)
  • code style changes
  • (anything I missed?)

I think we should maybe focus on getting this stuff merged in piece by piece.

— Reply to this email directly or view it on GitHub https://github.com/amitava82/angular-multiselect/pull/78#issuecomment-168541719 .

amitava82 commented 8 years ago

This PR has lots going on and there are files committed not relevant for the project (.idea). Indentations are overshadowing the actual changes. I'd appreciate a clean and legible commits.

zachlysobey commented 8 years ago

I'll try to get a chance in the next week or so to tackle this.