Pagination - Fix missing @currentPageSize argument in the "Compact" variant with SizeSelector

.changeset/light-clouds-explain.md

@@ -0,0 +1,5 @@
+---
+"@hashicorp/design-system-components": patch
+---
+
+`Pagination` - updated the logic for “Compact” variant to expose `@currentPageSize` and handle controlled/uncontrolled changes

packages/components/addon/components/hds/pagination/compact/index.js

@@ -4,6 +4,7 @@
  */
 
 import Component from '@glimmer/component';
+import { tracked } from '@glimmer/tracking';
 import { action } from '@ember/object';
 import { assert } from '@ember/debug';
 import { inject as service } from '@ember/service';
@@ -15,6 +16,18 @@ export const DEFAULT_PAGE_SIZES = [10, 30, 50];
 export default class HdsPaginationCompactIndexComponent extends Component {
   @service router;
 
+  // This private variable is used to differentiate between
+  // "uncontrolled" component (where the state is handled internally) and
+  // "controlled" component (where the state is handled externally, by the consumer's code).
+  // In the first case, the variable stores the internal state of the component at any moment,
+  // and its value is updated internally according to the user's interaction with the component.
+  // In the second case, the variable stores *only* the initial state of the component (coming from the arguments)
+  // at rendering time, but from that moment on it's not updated anymore, no matter what interaction the user
+  // has with the component (the state is controlled externally, eg. via query parameters)
+  @tracked _currentPageSize = this.args.currentPageSize ?? this.pageSizes[0];
+  @tracked isControlled;
+
+  showLabels = this.args.showLabels ?? true; // if the labels for the "prev/next" controls are visible
   showSizeSelector = this.args.showSizeSelector ?? false; // if the "size selector" block is visible
 
   constructor() {
@@ -31,28 +44,16 @@ export default class HdsPaginationCompactIndexComponent extends Component {
     // initialized and updated using the arguments passed to it.
 
     if (queryFunction === undefined) {
-      this.hasRouting = false;
+      this.isControlled = false;
     } else {
       assert(
         '@queryFunction for "Hds::Pagination::Numbered" must be a function',
         typeof queryFunction === 'function'
       );
-      this.hasRouting = true;
+      this.isControlled = true;
     }
   }
 
-  /**
-   * @param showLabels
-   * @type {boolean}
-   * @default true
-   * @description Show the labels for the "prev/next" controls
-   */
-  get showLabels() {
-    let { showLabels = true } = this.args;
-
-    return showLabels;
-  }
-
   /**
    * @param ariaLabel
    * @type {string}
@@ -62,6 +63,35 @@ export default class HdsPaginationCompactIndexComponent extends Component {
     return this.args.ariaLabel ?? 'Pagination';
   }
 
+  // This very specific `get/set` pattern is used to handle the two different use cases of the component
+  // being "controlled" (when it has routing, meaning it needs to support pagination controls as links/`LinkTo`)
+  // vs being "uncontrolled" (see comments above for details).
+  //
+  // If it has routing (and so it's "controlled"), than the value ("state") of the `currentPageSize` variable
+  // is *always* determined by the controller via arguments (most of the times, connected to query parameters in the URL).
+  // For this reason the "get" method always returns the value from the `args`,
+  // while the "set" method never updates the private internal state (_variable).
+  //
+  // If instead it doesn't have routing (and so it's "uncontrolled") than the value ("state") of the `currentPageSize` variables
+  // is *always* determined by the component's internal logic (and updated according to the user interaction with it).
+  // For this reason the "get" and "set" methods always read from or write to the private internal state (_variable).
+
+  get currentPageSize() {
+    if (this.isControlled) {
+      return this.args.currentPageSize;
+    } else {
+      return this._currentPageSize;
+    }
+  }
+
+  set currentPageSize(value) {
+    if (this.isControlled) {
+      // noop
+    } else {
+      this._currentPageSize = value;
+    }
+  }
+
   /**
    * @param pageSizes
    * @type {array of numbers}
@@ -83,9 +113,9 @@ export default class HdsPaginationCompactIndexComponent extends Component {
     return this.router.currentRoute?.queryParams || {};
   }
 
-  buildQueryParamsObject(page) {
-    if (this.hasRouting) {
-      return this.args.queryFunction(page, this.currentPage);
+  buildQueryParamsObject(page, pageSize) {
+    if (this.isControlled) {
+      return this.args.queryFunction(page, pageSize);
     } else {
       return {};
     }
@@ -100,9 +130,15 @@ export default class HdsPaginationCompactIndexComponent extends Component {
     };
 
     // the "query" is dynamic and needs to be calculated
-    if (this.hasRouting) {
-      routing.queryPrev = this.buildQueryParamsObject('prev');
-      routing.queryNext = this.buildQueryParamsObject('next');
+    if (this.isControlled) {
+      routing.queryPrev = this.buildQueryParamsObject(
+        'prev',
+        this.currentPageSize
+      );
+      routing.queryNext = this.buildQueryParamsObject(
+        'next',
+        this.currentPageSize
+      );
     } else {
       routing.queryPrev = undefined;
       routing.queryNext = undefined;
@@ -121,4 +157,23 @@ export default class HdsPaginationCompactIndexComponent extends Component {
       onPageChange(newPage);
     }
   }
+
+  @action
+  onPageSizeChange(newPageSize) {
+    let { onPageSizeChange } = this.args;
+
+    // we need to manually update the query parameters in the route (it's not a link!)
+    if (this.isControlled) {
+      // we pass `null` as value for the `page` argument, so consumers can handle this condition accordingly (probably will just change the side of the data/array slice)
+      const queryParams = this.buildQueryParamsObject(null, newPageSize);
+      this.router.transitionTo({ queryParams });
+    } else {
+      this.currentPageSize = newPageSize;
+    }
+
+    // invoke the callback function
+    if (typeof onPageSizeChange === 'function') {
+      onPageSizeChange(newPageSize);
+    }
+  }
 }

packages/components/addon/components/hds/pagination/numbered/index.js

@@ -85,6 +85,7 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
   // has with the component (the state is controlled externally, eg. via query parameters)
   @tracked _currentPage = this.args.currentPage ?? 1;
   @tracked _currentPageSize = this.args.currentPageSize ?? this.pageSizes[0];
+  @tracked isControlled;
 
   showInfo = this.args.showInfo ?? true; // if the "info" block is visible
   showLabels = this.args.showLabels ?? false; // if the labels for the "prev/next" controls are visible
@@ -107,7 +108,7 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
     // initialized and updated using the arguments passed to it.
 
     if (queryFunction === undefined) {
-      this.hasRouting = false;
+      this.isControlled = false;
     } else {
       assert(
         '@queryFunction for "Hds::Pagination::Numbered" must be a function',
@@ -118,7 +119,7 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
         typeof this.args.currentPageSize === 'number' &&
           typeof this.args.currentPage === 'number'
       );
-      this.hasRouting = true;
+      this.isControlled = true;
     }
 
     assert(
@@ -137,7 +138,7 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
   }
 
   // This very specific `get/set` pattern is used to handle the two different use cases of the component
-  // being "controlled" (when it has routing, meaning it needs to support links as controls)
+  // being "controlled" (when it has routing, meaning it needs to support pagination controls as links/`LinkTo`)
   // vs being "uncontrolled" (see comments above for details).
   //
   // If it has routing (and so it's "controlled"), than the value ("state") of the `currentPage/currentPageSize` variables
@@ -150,31 +151,31 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
   // For this reason the "get" and "set" methods always read from or write to the private internal state (_variable).
 
   get currentPage() {
-    if (this.hasRouting) {
+    if (this.isControlled) {
       return this.args.currentPage;
     } else {
       return this._currentPage;
     }
   }
 
   set currentPage(value) {
-    if (this.hasRouting) {
+    if (this.isControlled) {
       // noop
     } else {
       this._currentPage = value;
     }
   }
 
   get currentPageSize() {
-    if (this.hasRouting) {
+    if (this.isControlled) {
       return this.args.currentPageSize;
     } else {
       return this._currentPageSize;
     }
   }
 
   set currentPageSize(value) {
-    if (this.hasRouting) {
+    if (this.isControlled) {
       // noop
     } else {
       this._currentPageSize = value;
@@ -242,7 +243,7 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
   }
 
   buildQueryParamsObject(page, pageSize) {
-    if (this.hasRouting) {
+    if (this.isControlled) {
       return this.args.queryFunction(page, pageSize);
     } else {
       return {};
@@ -258,7 +259,7 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
     };
 
     // the "query" is dynamic and needs to be calculated
-    if (this.hasRouting) {
+    if (this.isControlled) {
       routing.queryPrev = this.buildQueryParamsObject(
         this.currentPage - 1,
         this.currentPageSize
@@ -323,10 +324,9 @@ export default class HdsPaginationNumberedIndexComponent extends Component {
     let { onPageSizeChange } = this.args;
 
     // we need to manually update the query parameters in the route (it's not a link!)
-    // notice: we agreed to reset the pagination to the first element (any alternative would result in an unpredictable UX)
-    if (this.hasRouting) {
-      let queryParams = Object.assign({}, this.routeQueryParams);
-      queryParams = this.buildQueryParamsObject(1, newPageSize);
+    if (this.isControlled) {
+      // notice: we agreed to reset the pagination to the first element (any alternative would result in an unpredictable UX)
+      const queryParams = this.buildQueryParamsObject(1, newPageSize);
       this.router.transitionTo({ queryParams });
     } else {
       this.currentPage = 1;

packages/components/tests/acceptance/components/hds/pagination-test.js

@@ -72,27 +72,126 @@ module('Acceptance | Component | hds/pagination', function (hooks) {
     await visit('/components/pagination');
 
     assert.strictEqual(currentURL(), '/components/pagination');
-
     assert
       .dom(
         '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr'
       )
       .exists({ count: 5 });
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr .hds-table__td'
+      )
+      .hasText('1');
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr:nth-child(5) .hds-table__td'
+      )
+      .hasText('5');
+
+    // CLICK "NEXT"
+    // ------------
 
     await click(
       '#demo4-compact-with-routing .hds-pagination .hds-pagination-nav__arrow--direction-next'
     );
+
     assert.strictEqual(
       currentURL(),
       '/components/pagination?demoExtraParam=hello&nextCursor_demo4=bmV4dF9fNg%3D%3D'
     );
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr'
+      )
+      .exists({ count: 5 });
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr .hds-table__td'
+      )
+      .hasText('6');
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr:nth-child(5) .hds-table__td'
+      )
+      .hasText('10');
+
+    // CHANGE PAGE SIZE
+    // ----------------
+
+    await select(
+      '#demo4-compact-with-routing .hds-pagination .hds-pagination-size-selector select',
+      '10'
+    );
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr'
+      )
+      .exists({ count: 10 });
+    assert.strictEqual(
+      currentURL(),
+      '/components/pagination?currentPageSize_demo4=10&demoExtraParam=hello&nextCursor_demo4=bmV4dF9fNg%3D%3D'
+    );
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr .hds-table__td'
+      )
+      .hasText('6');
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr:nth-child(10) .hds-table__td'
+      )
+      .hasText('15');
+
+    // CLICK "NEXT" + "PREV"
+    // ------------
+
+    await click(
+      '#demo4-compact-with-routing .hds-pagination .hds-pagination-nav__arrow--direction-next'
+    );
+    await click(
+      '#demo4-compact-with-routing .hds-pagination .hds-pagination-nav__arrow--direction-prev'
+    );
+    assert.strictEqual(
+      currentURL(),
+      '/components/pagination?currentPageSize_demo4=10&demoExtraParam=hello&prevCursor_demo4=cHJldl9fMTY%3D'
+    );
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr .hds-table__td'
+      )
+      .hasText('6');
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr:nth-child(10) .hds-table__td'
+      )
+      .hasText('15');
+
+    // CLICK "PREV"
+    // ------------
+    // this is a special test to make sure that when the prev cursor is less than the page size the demo code still works (there was a bug before)
 
     await click(
       '#demo4-compact-with-routing .hds-pagination .hds-pagination-nav__arrow--direction-prev'
     );
     assert.strictEqual(
       currentURL(),
-      '/components/pagination?demoExtraParam=hello&prevCursor_demo4=cHJldl9fNg%3D%3D'
+      '/components/pagination?currentPageSize_demo4=10&demoExtraParam=hello&prevCursor_demo4=cHJldl9fNg%3D%3D'
     );
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr'
+      )
+      // notice: even if the "page size" is 10, we see only 5 records because we are counting "10 records before record #6" and so only 5 records exist
+      .exists({ count: 5 });
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr .hds-table__td'
+      )
+      .hasText('1');
+    assert
+      .dom(
+        '#demo4-compact-with-routing .hds-table .hds-table__tbody .hds-table__tr:nth-child(5) .hds-table__td'
+      )
+      .hasText('5');
   });
 });