Skip to content

Replace map with repeat directive for rows rendering#43

Merged
dcharles525 merged 2 commits into
mainfrom
fix/table-loading
May 26, 2026
Merged

Replace map with repeat directive for rows rendering#43
dcharles525 merged 2 commits into
mainfrom
fix/table-loading

Conversation

@dcharles525

@dcharles525 dcharles525 commented May 26, 2026

Copy link
Copy Markdown
Member

This allows table updates without resetting any elements in the context of the row, like a kabob menu.

Summary

When rows data updates (e.g. from a polling interval), Lit's .map() destroys and recreates all row
DOM nodes on every render cycle. Any stateful child element inside a row, such as an open
trailhand-action-menu loses its state because the element is replaced with a new instance.

Replacing .map() with Lit's repeat() directive keys each row to row[this.keyField] (defaults to
'id'). Lit reuses existing DOM nodes for rows whose key hasn't changed, only creating/destroying nodes
for rows that are genuinely added or removed. Stateful child elements survive data refreshes.


Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ⚡ Performance improvement

Changes Made

Primary Changes

  • Replace .map() with repeat() directive in the row rendering section of DataTable.render()
  • Add import { repeat } from 'lit/directives/repeat.js'

Secondary/Collateral Changes

  • None

Technical Notes

Components:

  • src/components/data-table/data-table.ts row rendering changed from .map() to
    repeat(this._paginatedRows, (row) => row[this.keyField], (row) => html\...`)`

Implementation Details

Lit's .map() treats every re-render as a full replacement with no concept of row identity. repeat()
accepts a key function as its second argument and uses it to match old nodes to new data. When the key
is stable (same row ID), the existing <tr> and all its children are updated in place rather than
recreated.

The key function uses row[this.keyField], the existing public keyField property (default: 'id',
configurable via the key-field attribute). No new API surface introduced.

lit/directives/repeat.js is part of the lit package already present as a dependency, no new
packages added.


Testing

How to Test

  1. Render a trailhand-table with a renderActions prop that returns a trailhand-action-menu
  2. Open an action menu on any row
  3. Trigger a data refresh by updating the rows prop with the same rows (simulating a poll)
  4. Expected: the action menu remains open; the row is not recreated

Test Coverage

  • Unit tests added/updated
  • Integration tests added/updated
  • E2E tests added/updated
  • Manual testing completed

Browsers Tested

  • Chrome
  • Firefox
  • Safari
  • Edge

Potential Regressions

  • Row rendering in all table instances, verify rows still render correctly with sorting, filtering, and
    pagination active
  • Any consumer passing a key-field attribute should verify their key values are unique per row

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have updated documentation as needed
  • My changes generate no new warnings or errors
  • I have tested my changes locally
  • Any dependent changes have been merged and published

Additional Context

This fix was identified while integrating trailhand-table into a polling-driven application list view.
The table receives fresh rows data every 30 seconds. Without repeat(), every poll cycle caused all
open action menus to close because their DOM nodes were destroyed. The repeat() directive resolves
this by preserving node identity across renders.

This allows table updates without resetting any elements in the context of the row, like a kabob menu.
@dcharles525 dcharles525 marked this pull request as ready for review May 26, 2026 14:40
@dcharles525 dcharles525 requested a review from Hannahbird May 26, 2026 14:42

@Hannahbird Hannahbird left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dcharles525 dcharles525 merged commit f90a58a into main May 26, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants