Skip to content
This repository has been archived by the owner on Jan 7, 2022. It is now read-only.

Added internal paging #51

Merged
merged 7 commits into from
Jan 10, 2017
Merged

Added internal paging #51

merged 7 commits into from
Jan 10, 2017

Conversation

bensgroi
Copy link
Collaborator

@bensgroi bensgroi commented Jan 9, 2017

Added the internal paging feature, so sorting now works on the paging demo.
Disabled sorting on the virtual paging demo, since built-in sorting is incompatible at this time.

Works to include fix for #33

@codecov-io
Copy link

codecov-io commented Jan 9, 2017

Current coverage is 27.51% (diff: 63.15%)

Merging #51 into master will increase coverage by 7.52%

@@             master        #51   diff @@
==========================================
  Files            45         45          
  Lines          1321       1334    +13   
  Methods         169        169          
  Messages          0          0          
  Branches        307        313     +6   
==========================================
+ Hits            264        367   +103   
+ Misses         1057        967    -90   
  Partials          0          0          

Powered by Codecov. Last update 9ff1bcb...e75443b

@jonshaffer
Copy link
Owner

This looks good. Can you add some a couple unit tests and if you're feeling ambitious a protractor test for sorting+paging on the demo?

@jonshaffer jonshaffer added the bug label Jan 9, 2017
@jonshaffer jonshaffer added this to the 0.8.0 milestone Jan 9, 2017
@jonshaffer jonshaffer assigned bensgroi and unassigned bensgroi Jan 9, 2017
Copy link
Owner

@jonshaffer jonshaffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use some related unit tests, bodyController already has a spec started too which should make that easier

});
}, 200)
};
<head>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you tabbed <head at the same level of <html?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, VS Code doesn't do a great job at auto-formatting. I'll fix.

{ name: "Name", prop: "name" },
{ name: "Gender", prop: "gender" },
{ name: "Company", prop: "company" }
{ name: "Name", prop: "name", sortable: false },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we could use a global 'sortable'!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Added issue #53

let idxs = this.getFirstLastIndexes(),
idx = idxs.first;

this.tempRows.splice(0, this.tempRows.length);
while (idx < idxs.last) {
this.tempRows.push(rows[idx++])
}
} else if (this.options.paging.size) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is nested in a this.options.paging.size already, which would prevent the next else statement from ever firing. Looks safe to remove the if condition and the else below it, but you may know more about this area than me now :)

That being said - do we know the relation of the externalPaging logic to all of that paging that happens in virtual paging?

Copy link
Owner

@jonshaffer jonshaffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solid additions here

@bensgroi bensgroi merged commit 4e17de4 into master Jan 10, 2017
@bensgroi bensgroi deleted the internal-paging branch January 10, 2017 19:07
@jonshaffer jonshaffer restored the internal-paging branch January 12, 2017 15:58
@jonshaffer jonshaffer deleted the internal-paging branch January 12, 2017 16:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants