Skip to content

Commit 735a63f

Browse files
KristinLBradleyKristinLBradley
committed
HDS-4273 Add temporary test code for testing bug, add missing types, add default value for identityKey
1 parent 62935d5 commit 735a63f

File tree

4 files changed

+131
-4
lines changed

4 files changed

+131
-4
lines changed

‎packages/components/src/components/hds/table/index.ts

+20-4
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ export default class HdsTable extends Component<HdsTableSignature> {
105105
get getSortCriteria(): string | HdsTableSortingFunction<unknown> {
106106
// get the current column
107107
const currentColumn = this.args?.columns?.find(
108-
(column) => column.key === this.sortBy
108+
(column): boolean => column.key === this.sortBy
109109
);
110110
if (
111111
// check if there is a custom sorting function associated with the current `sortBy` column (we assume the column has `isSortable`)
@@ -124,7 +124,9 @@ export default class HdsTable extends Component<HdsTableSignature> {
124124
if (this.args.identityKey === 'none') {
125125
return undefined;
126126
} else {
127-
return this.args.identityKey ?? '@identity';
127+
// We return a default key to cause an update operation instead of replacement when the model changes to side-step
128+
// a bug in Ember which would cause the table to lose its selection state
129+
return this.args.identityKey ?? 'hds-table-key';
128130
}
129131
}
130132

@@ -289,6 +291,13 @@ export default class HdsTable extends Component<HdsTableSignature> {
289291
checkbox: HdsFormCheckboxBaseSignature['Element'],
290292
selectionKey?: string
291293
): void {
294+
// TEST:
295+
console.log(
296+
`[${this.args.caption}] Did insert row checkbox`,
297+
selectionKey,
298+
this._selectableRows.map((row) => row.selectionKey)
299+
);
300+
292301
if (selectionKey) {
293302
this._selectableRows.push({ selectionKey, checkbox });
294303
}
@@ -297,8 +306,15 @@ export default class HdsTable extends Component<HdsTableSignature> {
297306

298307
@action
299308
willDestroyRowCheckbox(selectionKey?: string): void {
309+
// TEST:
310+
console.log(
311+
`[${this.args.caption}] Will destroy row checkbox`,
312+
selectionKey,
313+
this._selectableRows.map((row) => row.selectionKey)
314+
);
315+
300316
this._selectableRows = this._selectableRows.filter(
301-
(row) => row.selectionKey !== selectionKey
317+
(row): boolean => row.selectionKey !== selectionKey
302318
);
303319
this.setSelectAllState();
304320
}
@@ -308,7 +324,7 @@ export default class HdsTable extends Component<HdsTableSignature> {
308324
if (this._selectAllCheckbox) {
309325
const selectableRowsCount = this._selectableRows.length;
310326
const selectedRowsCount = this._selectableRows.filter(
311-
(row) => row.checkbox.checked
327+
(row): boolean => row.checkbox.checked
312328
).length;
313329

314330
this._selectAllCheckbox.checked =

‎showcase/app/controllers/components/table-multi-select-bug.js

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/**
2+
* Copyright (c) HashiCorp, Inc.
3+
* SPDX-License-Identifier: MPL-2.0
4+
*/
5+
6+
import Controller from '@ember/controller';
7+
import { tracked } from '@glimmer/tracking';
8+
import { action } from '@ember/object';
9+
10+
// prettier-ignore
11+
const daysOfChristmas = [
12+
{ key: 'one', quantity: 1, type: 'bird', line: 'A partridge in a pear tree' },
13+
{ key: 'two', quantity: 2, type: 'bird', line: 'Two turtle doves' },
14+
{ key: 'three', quantity: 3, type: 'bird', line: 'Three french hens' },
15+
{ key: 'four', quantity: 4, type: 'bird', line: 'Four calling birds' },
16+
{ key: 'five', quantity: 5, type: 'jewelry', line: 'Five gold rings' },
17+
{ key: 'six', quantity: 6, type: 'more bird', line: 'Six geese a-laying' },
18+
{ key: 'seven', quantity: 7, type: 'bird again', line: 'Seven swans a-swimming' },
19+
{ key: 'eight', quantity: 8, type: 'servant', line: 'Eight maids a-milking' },
20+
{ key: 'nine', quantity: 9, type: 'maybe royalty', line: 'Nine ladies dancing' },
21+
{ key: 'ten', quantity: 10, type: 'definitely royalty', line: 'Ten lords a-leaping' },
22+
{ key: 'eleven', quantity: 11, type: 'musician', line: 'Eleven pipers piping' },
23+
{ key: 'twelve', quantity: 12, type: 'musician, technically', line: 'Twelve drummers drumming' },
24+
];
25+
26+
export default class extends Controller {
27+
@tracked showTable = false;
28+
@tracked lyrics = daysOfChristmas.slice();
29+
30+
@action dirtyModel() {
31+
// Deep copy to destroy references
32+
this.lyrics = this.lyrics.map((r) => ({ ...r }));
33+
}
34+
35+
printSelection = (selection) =>
36+
console.log('Printing selection:', selection.selectedRowsKeys);
37+
}

‎showcase/app/router.ts

+1
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ Router.map(function () {
6262
this.route('side-nav');
6363
this.route('stepper');
6464
this.route('table');
65+
this.route('table-multi-select-bug');
6566
this.route('tag');
6667
this.route('text');
6768
this.route('time');

‎showcase/app/templates/components/table-multi-select-bug.hbs

+73
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
{{!
2+
Copyright (c) HashiCorp, Inc.
3+
SPDX-License-Identifier: MPL-2.0
4+
}}
5+
6+
{{page-title "Table multi-select bug"}}
7+
8+
<Shw::Text::H1>Table multi-select bug</Shw::Text::H1>
9+
10+
<section>
11+
<p>When a table isn't immediately rendered, a bug occurs where table rows render twice. Normally this is not a big
12+
deal, but due to the way that row-level checkboxes are registered, we get this unfortunate sequence of events:</p>
13+
<ol>
14+
<li><code>didInsertRowCheckbox</code>: Insert selection keys to <code>_selectableRows</code></li>
15+
<li><code>didInsertRowCheckbox</code>: Insert selection keys to
16+
<code>_selectableRows</code>
17+
a second time. This results in an array that's 2x of every selection key.</li>
18+
<li><code>willDestroyRowCheckbox</code>: Filters
19+
<code>_selectableRows</code>
20+
by removed selection key, except this removes the first and second insertions resulting in an empty array.</li>
21+
</ol>
22+
23+
<Shw::Text::H2>This will work until you click the "Force new data" button</Shw::Text::H2>
24+
25+
<Hds::Button @text="Force new data" {{on "click" this.dirtyModel}} />
26+
27+
<Hds::Table
28+
@model={{this.lyrics}}
29+
@caption="local property"
30+
@columns={{array
31+
(hash key="quantity" label="Quantity")
32+
(hash key="type" label="Type")
33+
(hash key="line" label="Line")
34+
}}
35+
@isSelectable={{true}}
36+
@onSelectionChange={{this.printSelection}}
37+
>
38+
<:body as |B|>
39+
<B.Tr @selectionKey={{B.data.key}} @selectionAriaLabelSuffix=" {{B.data.key}}">
40+
<B.Td>{{B.data.quantity}}</B.Td>
41+
<B.Td>{{B.data.type}}</B.Td>
42+
<B.Td>{{B.data.line}}</B.Td>
43+
</B.Tr>
44+
</:body>
45+
</Hds::Table>
46+
47+
<Shw::Text::H2>This will always work because
48+
<code>@identityKey</code>
49+
performs an update not a replace.</Shw::Text::H2>
50+
51+
<Hds::Button @text="Force new data" {{on "click" this.dirtyModel}} />
52+
53+
<Hds::Table
54+
@model={{this.lyrics}}
55+
@caption="local property"
56+
@columns={{array
57+
(hash key="quantity" label="Quantity")
58+
(hash key="type" label="Type")
59+
(hash key="line" label="Line")
60+
}}
61+
@identityKey="key"
62+
@isSelectable={{true}}
63+
@onSelectionChange={{this.printSelection}}
64+
>
65+
<:body as |B|>
66+
<B.Tr @selectionKey={{B.data.key}} @selectionAriaLabelSuffix=" {{B.data.key}}">
67+
<B.Td>{{B.data.quantity}}</B.Td>
68+
<B.Td>{{B.data.type}}</B.Td>
69+
<B.Td>{{B.data.line}}</B.Td>
70+
</B.Tr>
71+
</:body>
72+
</Hds::Table>
73+
</section>

0 commit comments

Comments
 (0)