fix(tile-group): disallow multiple selected attributes on single selection modes #9284
Conversation
…gle selection modes
github-actions
bot
added
the
bug
josercarcamo
changed the title
packages/calcite-components/src/components/tile-group/tile-group.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks great, thanks @josercarcamo ! Just left a suggestion that can save on an unnecessary extra re-render that occurs when setting selectedItems.
eriklharper
added
the
low risk
eriklharper
added
the
pr ready for visual snapshots
| @@ -492,5 +492,41 @@ describe("calcite-tile-group", () => { | |||
|
|
|||
| await selectedItemAsserter([]); | |||
| }); | |||
|
|
|||
| it("single selection mode allows only one tile with selected attribute", async () => { | |||
There was a problem hiding this comment.
@josercarcamo These new tests share a bit of code, so could you look into DRYing them up?
| @@ -151,7 +151,20 @@ export class TileGroup implements InteractiveComponent, SelectableGroupComponent | |||
| }; | |||
|
|
|||
| private updateSelectedItems = (): void => { | |||
| this.selectedItems = this.items?.filter((el) => el.selected); | |||
| const selectedItems = this.items?.filter((el) => el.selected); | |||
There was a problem hiding this comment.
@eriklharper Can items be null? It seems like it's guaranteed as it's initialized as [] and I think getSlottedTiles would always return an array. This would help us remove a lot of guards in the component.
There was a problem hiding this comment.
Yes, this.items can be undefined since its populated by the slotted tiles and that can't always be guaranteed. getSlottedTiles doesn't do anything extra to prevent from returning undefined in some cases, but we could add that in I suppose.
There was a problem hiding this comment.
I think it's worth getting rid of those guards by updating getSlottedTiles to always return an array.
| ) { | ||
| this.selectedItems = [selectedItems.pop()]; | ||
| this.items?.forEach((el) => { | ||
| if (this.selectedItems.indexOf(el) === -1) { |
There was a problem hiding this comment.
Suggestion to simplify: if (this.selectedItems.includes(el)) {
| } | ||
| }); | ||
| } else { | ||
| this.selectedItems = selectedItems ?? []; |
There was a problem hiding this comment.
If items is guaranteed, we can eliminate the fallback empty array.
geospatialem
modified the milestones:
2024-05-14 - May maintenance release,
2024-05-28 - May Main Release
Related Issue: #9271
Summary
Displayed only the last tile as selected when multiple tiles have the selected attribute.