Skip to content
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

[NG] Fix pagination init lifecycle #3175

Merged
merged 1 commit into from Mar 7, 2019

Conversation

Projects
None yet
4 participants
@youdz
Copy link
Contributor

youdz commented Mar 5, 2019

Because of a lifecycle quirk, we would instantiate as many ClrDatagridRow components
as we would have in the entire dataset. They wouldn't go throught their full Angular
lifecycle, but they would be instantiated nonetheless.
This commit fixes this by making the Items provider wait for the page size to be
available before emitting any changes.

Fixes #3082.

@youdz youdz added Bug Dev High Oops labels Mar 5, 2019

@youdz youdz added this to the 1.1.1 milestone Mar 5, 2019

@youdz youdz requested a review from Jinnie Mar 5, 2019

@hippee-lee
Copy link
Contributor

hippee-lee left a comment

Just the one question about the new public property in Page.

I do have a general comment about overloaded selectors. I have mixed feelings about them. When I read the integration test I had to read it twice before I realized how it was working b/c I scanned past the selector right to the class. I know we use the pattern in several different place but it always makes me stop and think b/c I haven't memorized which ones are overloaded.

@@ -12,6 +12,8 @@ import { StateDebouncer } from './state-debouncer.provider';
export class Page {
constructor(private stateDebouncer: StateDebouncer) {}

public activated = false;

This comment has been minimized.

@hippee-lee

hippee-lee Mar 5, 2019

Contributor

Since this is public, should it have a test?

This comment has been minimized.

@gnomeontherun

gnomeontherun Mar 6, 2019

Contributor

The property itself is not important to test, but the side effects of it being set. I think the test covers the cases except when changePage is triggered.

@@ -178,7 +178,8 @@ export class Items<T = any> {
* Extracts the current page from the sorted list
*/
private _changePage() {
if (this.uninitialized) {
// If we know we have pagination but the page size hasn't been set yet, we wait for it.
if (this.uninitialized || (this._page.activated && this._page.size === 0)) {

This comment has been minimized.

@gnomeontherun

gnomeontherun Mar 6, 2019

Contributor

Do we have a test to cover this? I haven't looked recently, but this is a new code path that we could extend an existing test to validate.

@youdz

This comment has been minimized.

Copy link
Contributor Author

youdz commented Mar 6, 2019

@hippee-lee: You're right, in this case the selector overload is pointless, my bad. I'm switching to an attribute directive since I control the test component anyway.

For our components though, we sadly don't have much choice since Angular doesn't offer a good composition pattern yet. But I understand the concern, maybe we should think of a way to document overloads somewhere in the code.

@youdz youdz force-pushed the youdz:topic/datagrid-pagination-lifecycle branch from b5e1e3b to 62e434f Mar 6, 2019

[NG] Fix pagination init lifecycle
Because of a lifecycle quirk, we would instantiate as many ClrDatagridRow components
as we would have in the entire dataset. They wouldn't go throught their full Angular
lifecycle, but they would be instantiated nonetheless.
This commit fixes this by making the Items provider wait for the page size to be
available before emitting any changes.

Signed-off-by: Eudes Petonnet-Vincent <epetonnetvince@vmware.com>

@youdz youdz force-pushed the youdz:topic/datagrid-pagination-lifecycle branch from 62e434f to 31e6dcb Mar 7, 2019

@youdz youdz merged commit 329bf04 into vmware:master Mar 7, 2019

1 of 2 checks passed

deploy/netlify Deploy preview failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.