Performance improvement

[arindrajitb]

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]

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, 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.

[arindrajitb]

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 [email protected] 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
#78 (comment)
.

[arindrajitb]

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 [email protected] 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 [email protected] 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
#78 (comment)
.

[arindrajitb]

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 [email protected] 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 [email protected] 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 [email protected]
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
#78 (comment)
.

[amitava82]

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]

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