Skip to content

Commit

Permalink
fix(data-table): do not overwrite row.cells property (#1336)
Browse files Browse the repository at this point in the history
* refactor(data-table): remove unneeded third argument from resolvePath call

* fix(data-table): do not overwrite `cells` property (#667)

* perf(data-table): early return if path in object when resolving path
  • Loading branch information
metonym committed Jun 5, 2022
1 parent 260bf4e commit d2cdb8e
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 16 deletions.
36 changes: 21 additions & 15 deletions src/DataTable/DataTable.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,13 @@
.map(({ key }, i) => ({ key, id: key }))
.reduce((a, c) => ({ ...a, [c.key]: c.id }), {})
);
const resolvePath = (object, path) =>
path
const resolvePath = (object, path) => {
if (path in object) return object[path];
return path
.split(/[\.\[\]\'\"]/)
.filter((p) => p)
.reduce((o, p) => (o && typeof o === "object" ? o[p] : o), object);
};
setContext("DataTable", {
sortHeader,
Expand Down Expand Up @@ -195,14 +197,18 @@
$: if (radio || batchSelection) selectable = true;
$: tableSortable.set(sortable);
$: headerKeys = headers.map(({ key }) => key);
$: $tableRows = rows.map((row) => ({
...row,
cells: headerKeys.map((key, index) => ({
key,
value: resolvePath(row, key),
display: headers[index].display,
})),
}));
$: tableCellsByRowId = rows.reduce(
(rows, row) => ({
...rows,
[row.id]: headerKeys.map((key, index) => ({
key,
value: resolvePath(row, key),
display: headers[index].display,
})),
}),
{}
);

This comment has been minimized.

Copy link
@brunnerh

brunnerh Jun 5, 2022

Contributor

@metonym This seems a bit inefficient, because it recreates the whole dictionary for every row. When doing something like this I prefer just updating the existing object instead to prevent that. e.g.

rows.reduce(
    (rows, row) => {
        rows[row.id] = ...
        return rows;
    },
    {}
  )

Not sure at which table size this is going to make a difference though...

This comment has been minimized.

Copy link
@brunnerh

brunnerh Jun 5, 2022

Contributor

@metonym There is another alternative that avoids mutations:

Object.fromEntries(rows.map(row => [
   row.id,
   headerKeys.map((key, index) => ...)
]))

This comment has been minimized.

Copy link
@metonym

metonym Jun 5, 2022

Author Collaborator

Noted. This should be an easy win.

#1338

$: $tableRows = rows;
$: sortedRows = [...$tableRows];
$: ascending = $sortHeader.sortDirection === "ascending";
$: sortKey = $sortHeader.key;
Expand All @@ -213,11 +219,11 @@
} else {
sortedRows = [...$tableRows].sort((a, b) => {
const itemA = ascending
? resolvePath(a, sortKey, "")
: resolvePath(b, sortKey, "");
? resolvePath(a, sortKey)
: resolvePath(b, sortKey);
const itemB = ascending
? resolvePath(b, sortKey, "")
: resolvePath(a, sortKey, "");
? resolvePath(b, sortKey)
: resolvePath(a, sortKey);
if ($sortHeader.sort) return $sortHeader.sort(itemA, itemB);
Expand Down Expand Up @@ -464,7 +470,7 @@
{/if}
</td>
{/if}
{#each row.cells as cell, j (cell.key)}
{#each tableCellsByRowId[row.id] as cell, j (cell.key)}
{#if headers[j].empty}
<td class:bx--table-column-menu="{headers[j].columnMenu}">
<slot
Expand Down
2 changes: 1 addition & 1 deletion src/DataTable/ToolbarSearch.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
if (shouldFilterRows === true) {
rows = rows.filter((row) => {
return Object.entries(row)
.filter(([key]) => !["cells", "id"].includes(key))
.filter(([key]) => key !== "id")
.some(([key, _value]) => {
if (typeof _value === "string" || typeof _value === "number") {
return (_value + "")
Expand Down

0 comments on commit d2cdb8e

Please sign in to comment.