Skip to content

Commit 705681f

Browse files
Apply suggestions from code review
Co-authored-by: Cristiano Rastelli <cristiano.rastelli@hashicorp.com>
1 parent 25aa431 commit 705681f

File tree

2 files changed

+9
-5
lines changed

2 files changed

+9
-5
lines changed

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

+4-3
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,9 @@ export default class HdsTable extends Component<HdsTableSignature> {
124124
if (this.args.identityKey === 'none') {
125125
return undefined;
126126
} else {
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
127+
// We return `@identity` as default to cause an update operation instead of replacement when the model changes to avoid unexpected side effects
128+
// see: "Specifying Keys" here: https://api.emberjs.com/ember/6.1/classes/Ember.Templates.helpers/methods/each?anchor=each
129+
// see also: https://github.com/hashicorp/design-system/pull/1160 + https://github.com/hashicorp/cloud-ui/pull/5178 + https://hashicorp.atlassian.net/browse/HDS-4273
129130
return this.args.identityKey ?? '@identity';
130131
}
131132
}
@@ -314,7 +315,7 @@ export default class HdsTable extends Component<HdsTableSignature> {
314315
);
315316

316317
// Fix for selectableRows state not being maintained properly when model updates:
317-
// Delete the first row with the given selction key you find from the selectableRows array
318+
// Delete the first row with the given selection key you find from the selectableRows array
318319
// Search for the index of the row to delete. If found, delete it.
319320
// (Fixes issue of not rows not being properly selectable including "select all" functionality)
320321
const rowToDeleteIndex = this._selectableRows.findIndex(

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

+5-2
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,11 @@ export default class extends Controller {
2828
@tracked lyrics = daysOfChristmas.slice();
2929

3030
@action dirtyModel() {
31-
// Deep copy to destroy references
32-
this.lyrics = this.lyrics.map((r) => ({ ...r }));
31+
this.lyrics = this.lyrics
32+
// Deep copy to destroy references
33+
.map((r) => ({ ...r }))
34+
// Randomly sort to see what happens in this case
35+
.sort(() => Math.random() - 0.5);
3336
}
3437

3538
printSelection = (selection) =>

0 commit comments

Comments
 (0)