[70204] Create a new TableComponent based on Primer React's DataTable by bsatarnejad · Pull Request #409 · opf/primer_view_components

bsatarnejad · 2026-01-31T09:50:10Z

What are you trying to accomplish?

Create table and data table components

Closes https://community.openproject.org/wp/70204

changeset-bot · 2026-01-31T09:50:14Z

🦋 Changeset detected

Latest commit: 605565c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@openproject/primer-view-components	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

myabc

I haven't had time to give this a full review, but it would be good to go ahead and squash down the commits.

github-actions · 2026-02-18T00:38:23Z

⚠️ Visual or ARIA snapshot differences found

Our visual and ARIA snapshot tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review differences

Copilot

Pull request overview

This PR introduces new OpenProject-specific Table and DataTable ViewComponents (inspired by Primer React’s DataTable) including supporting subcomponents, previews, styling, JS behavior, tests, and updated generated metadata/static files.

Changes:

Add Primer::OpenProject::Table and subcomponents for building semantically-correct HTML tables.
Add Primer::OpenProject::DataTable with column definitions, sortable headers, CSS, and a Catalyst-based custom element for client-side sorting.
Add previews/tests and update generated static metadata + dependency bump for @github/catalyst.

Reviewed changes

Copilot reviewed 58 out of 59 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
test/css/component_specific_selectors_test.rb	Ignores DataTable selectors that are intentionally not present in previews.
test/components/primer/open_project/table/table_test.rb	Unit test for rendering basic table structure.
test/components/primer/open_project/table/row_test.rb	Unit tests for `Table::Row` rendering and cells.
test/components/primer/open_project/table/header_test.rb	Unit tests for `Table::Header` scope handling and validation.
test/components/primer/open_project/table/header_row_test.rb	Unit tests for `Table::HeaderRow` behavior.
test/components/primer/open_project/table/head_test.rb	Unit tests for `Table::Head` rendering behavior.
test/components/primer/open_project/table/foot_test.rb	Unit tests for `Table::Foot` rendering behavior.
test/components/primer/open_project/table/col_group_test.rb	Unit tests for `Table::ColGroup` rendering behavior.
test/components/primer/open_project/table/col_group/col_test.rb	Unit tests for `Table::ColGroup::Col`.
test/components/primer/open_project/table/cell_test.rb	Unit tests for `Table::Cell` content/text behavior.
test/components/primer/open_project/table/caption_test.rb	Unit tests for `Table::Caption` conditional rendering.
test/components/primer/open_project/table/body_test.rb	Unit tests for `Table::Body` rendering behavior.
test/components/primer/open_project/data_table/sort_header_test.rb	Unit tests for sortable header rendering and aria-sort behavior.
test/components/primer/open_project/data_table/data_table_test.rb	Unit tests for DataTable slots, a11y labeling, sort init, and grid-template.
test/components/primer/open_project/data_table/column_test.rb	Unit tests for column header/cell rendering behavior.
test/components/component_test.rb	Registers new components and smoke-renders them in the registry test.
static/statuses.json	Marks new components as `open_project`.
static/previews.json	Registers Lookbook previews for Table and DataTable.
static/info_arch.json	Adds documentation metadata entries for new components.
static/constants.json	Adds generated constants/slot metadata for new components.
static/classes.json	Tracks CSS class usage for new components.
static/audited_at.json	Adds audit placeholders for new components.
static/arguments.json	Adds generated argument docs entries for new components.
previews/primer/open_project/table_preview/playground.html.erb	Adds Table playground preview template.
previews/primer/open_project/table_preview/default.html.erb	Adds Table default preview template.
previews/primer/open_project/table_preview.rb	Adds Table preview class + playground params.
previews/primer/open_project/data_table_preview/with_row_actions.html.erb	Adds DataTable preview demonstrating row actions.
previews/primer/open_project/data_table_preview/playground.html.erb	Adds DataTable playground preview template.
previews/primer/open_project/data_table_preview/default.html.erb	Adds DataTable default preview template.
previews/primer/open_project/data_table_preview.rb	Adds DataTable preview class + playground params/sample data.
package.json	Bumps `@github/catalyst` dependency range.
package-lock.json	Updates lockfile for new Catalyst version.
app/components/primer/primer.ts	Includes the new DataTable custom element JS in the bundle.
app/components/primer/primer.pcss	Includes DataTable component CSS in the bundle.
app/components/primer/open_project/table/row_group.rb	Adds RowGroup base component for table sections.
app/components/primer/open_project/table/row_group.html.erb	Template for rendering RowGroup rows/content.
app/components/primer/open_project/table/row.rb	Adds table row component with typed cell slots.
app/components/primer/open_project/table/row.html.erb	Template for rendering a table row.
app/components/primer/open_project/table/header_row.rb	Adds header-row specialization of Row.
app/components/primer/open_project/table/header_row.html.erb	Template for rendering a header row.
app/components/primer/open_project/table/header.rb	Adds header-cell component with scope/role alignment.
app/components/primer/open_project/table/head.rb	Adds thead rowgroup component.
app/components/primer/open_project/table/foot.rb	Adds tfoot rowgroup component.
app/components/primer/open_project/table/col_group/col.rb	Adds col component for colgroup/cols.
app/components/primer/open_project/table/col_group.rb	Adds colgroup component.
app/components/primer/open_project/table/col_group.html.erb	Template for rendering colgroup/cols.
app/components/primer/open_project/table/cell.rb	Adds td cell component with alignment.
app/components/primer/open_project/table/caption.rb	Adds caption component with conditional render.
app/components/primer/open_project/table/body.rb	Adds tbody rowgroup component.
app/components/primer/open_project/table.rb	Adds the low-level Table primitive and its slots.
app/components/primer/open_project/table.html.erb	Template assembling caption/colgroups/head/bodies/foot.
app/components/primer/open_project/data_table_element.ts	Adds Catalyst custom element implementing client-side sorting.
app/components/primer/open_project/data_table/sort_header.rb	Adds sortable header component (aria-sort + styling).
app/components/primer/open_project/data_table/sort_header.html.erb	Sort header markup (button + icons + action hook).
app/components/primer/open_project/data_table/column.rb	Adds DataTable column definition component.
app/components/primer/open_project/data_table.rb	Adds DataTable main component logic (columns, headers, grid template, a11y).
app/components/primer/open_project/data_table.pcss	Adds DataTable styles (layout, density, sorting visuals).
app/components/primer/open_project/data_table.html.erb	Adds DataTable markup using `Table` primitive + sortable headers.
.changeset/stupid-lamps-promise.md	Adds a minor-release changeset entry for the new components.

Comments suppressed due to low confidence (1)

app/components/primer/open_project/data_table_element.ts:95

The custom element is registered as data-table-element, but the component markup/actions use data-table (e.g., click:data-table#toggleSort and <data-table> in data_table.html.erb). This mismatch prevents the controller from ever being instantiated. Register the element under the tag actually used in the templates (likely data-table, consistent with other elements like sub-header, page-header), or update the templates/actions to use data-table-element everywhere.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Replace renders_many class slot with a lambda that executes the config block at creation time, removing the .to_s side-effect hack from the template. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Prevents NoMethodError when RowGroup is instantiated directly, since render? calls rows.any?. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 61 out of 73 changed files in this pull request and generated 5 comments.

Copilot · 2026-04-25T20:48:18Z

+  render(Primer::OpenProject::Table.new(data: { controller: "table-highlighting" })) do |table|
+    table.with_caption { "Ice Cream Prices (Summer 2026)" }
+
+    table.with_colgroup do |colgroup|
+      colgroup.with_col


This preview sets data-controller: "table-highlighting", but there doesn't appear to be a corresponding controller/custom element implementation in this repo. If this is meant to demonstrate behavior, consider adding the implementation (or removing the attribute to avoid confusion).

Copilot · 2026-04-25T20:48:18Z

+  toggleSort(event: MouseEvent) {
+    const header = (event.target as Element).closest('th')!
+    const ariaSort = header.getAttribute('aria-sort')
+    const sortAscendingIcon = header.querySelector('.TableSortIcon--ascending')
+    const sortDescendingIcon = header.querySelector('.TableSortIcon--descending')
+
+    if (ariaSort === 'descending') {
+      header.setAttribute('aria-sort', 'ascending')
+      sortAscendingIcon?.classList.remove('d-none')
+      sortDescendingIcon?.classList.add('d-none')
+    } else {
+      header.setAttribute('aria-sort', 'descending')
+      sortDescendingIcon?.classList.remove('d-none')
+      sortAscendingIcon?.classList.add('d-none')
+    }
+
+    const siblings = Array.from(header.parentElement!.children).filter(
+      (el): el is HTMLElement => el !== header && el instanceof HTMLElement,
+    )
+
+    for (const sibling of siblings) {
+      resetSort(sibling)
+    }
+
+    sortTableByAriaSort(this.table)
+  }


toggleSort treats the initial unsorted state (no aria-sort attribute) the same as ascending and flips it to descending, which makes the first click sort descending even though the UI initially shows the ascending icon for NONE. Consider explicitly handling the null/none state and cycling NONE -> ascending -> descending (and updating icons/aria-sort accordingly).

Copilot · 2026-04-25T20:48:19Z

+  const sortedRows = rows.sort((a, b) => {
+    const aText = a.children[columnIndex].textContent?.trim() ?? ''
+    const bText = b.children[columnIndex].textContent?.trim() ?? ''
+
+    const aNum = parseFloat(aText)
+    const bNum = parseFloat(bText)
+
+    const valueA = isNaN(aNum) ? aText : aNum
+    const valueB = isNaN(bNum) ? bText : bNum
+
+    if (valueA < valueB) return direction === 'ascending' ? -1 : 1
+    if (valueA > valueB) return direction === 'ascending' ? 1 : -1
+    return 0


sortTableByAriaSort uses parseFloat to decide whether to treat a cell as numeric. This will incorrectly classify values like ISO dates (e.g., 2026-02-10) as the number 2026, causing sorting to be wrong/no-op for those columns. Consider using a stricter numeric check (e.g., regex for full-string numeric) or always sorting as strings with Intl.Collator (with {numeric: true}) unless an explicit numeric sort is requested.

Copilot · 2026-04-25T20:48:19Z

+.ButtonReset {
+    margin: 0;
+    display: inline-flex;
+    padding: 0;
+    border: 0;
+    appearance: none;
+    background: none;
+    cursor: pointer;
+    text-align: start;
+    font: inherit;
+    color: inherit;
+    align-items: center;
+}


.ButtonReset is a very generic, non-component-prefixed class name and is now globally available via primer.pcss. This increases the risk of styling collisions in consuming apps. Consider scoping it to the DataTable (e.g., .DataTable-ButtonReset or nesting under .Table/.TableHeader) so it only affects this component's markup.

Copilot · 2026-04-25T20:48:19Z

+<style>
+    table {
+        border: 1px solid black;
+    }
+


The preview injects global CSS (table { ... }) which can unintentionally affect any other table markup rendered in the preview page. Consider scoping these styles to a wrapper element/class for this preview (or using component classes) to avoid leaking styles.

github-actions Bot added the minor release label Jan 31, 2026

myabc reviewed Feb 2, 2026

View reviewed changes

Comment thread app/components/primer/open_project/data_table/column.rb

myabc self-requested a review February 3, 2026 23:48

myabc requested changes Feb 3, 2026

View reviewed changes

Comment thread app/components/primer/open_project/data_table_element.ts Outdated

Comment thread app/components/primer/open_project/data_table.html.erb Outdated

myabc requested a review from HDinger February 6, 2026 13:56

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from aee4a07 to c473f80 Compare February 18, 2026 00:04

myabc reviewed Feb 18, 2026

View reviewed changes

Comment thread app/components/primer/open_project/table/header_row.rb

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch 3 times, most recently from 573dc4e to 67554a5 Compare February 18, 2026 00:27

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from edc8b12 to 4764210 Compare February 18, 2026 01:12

myabc requested review from Copilot February 18, 2026 01:12

Copilot started reviewing on behalf of myabc February 18, 2026 01:13 View session

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 509dc34 to 6c3ae0e Compare February 18, 2026 01:13

Copilot AI reviewed Feb 18, 2026

View reviewed changes

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 1cf635f to d6d88cd Compare February 18, 2026 01:52

bsatarnejad and others added 3 commits February 17, 2026 23:03

[#70204] Introduce Table and DataTable components

0261415

Add changelog

f291b54

Generating component snapshots

f5e8f8a

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 36b9f27 to f5e8f8a Compare February 18, 2026 02:05

Copilot AI reviewed Feb 18, 2026

View reviewed changes

Generating component snapshots

fa9f7a6

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 2602f47 to 8655a51 Compare February 18, 2026 02:25

myabc and others added 2 commits February 17, 2026 23:31

Eager column config via lambda slot

a607f93

Replace renders_many class slot with a lambda that executes the config block at creation time, removing the .to_s side-effect hack from the template. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add default rows method to RowGroup base class

1e40dab

Prevents NoMethodError when RowGroup is instantiated directly, since render? calls rows.any?. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 8655a51 to 1e40dab Compare February 18, 2026 02:31

Generating component snapshots

605565c

myabc added ruby javascript work in progress Do not merge without further discussion labels Mar 8, 2026

myabc mentioned this pull request Mar 8, 2026

[#70204] Introduce DataTableComponent (port from Primer React) and unstyled TableComponent. opf/openproject#21532

Closed

3 tasks

bsatarnejad added work in progress Do not merge without further discussion and removed work in progress Do not merge without further discussion labels Mar 24, 2026

myabc requested a review from Copilot April 25, 2026 20:42

Copilot started reviewing on behalf of myabc April 25, 2026 20:42 View session

Copilot AI reviewed Apr 25, 2026

View reviewed changes

Conversation

bsatarnejad commented Jan 31, 2026

What are you trying to accomplish?

Uh oh!

changeset-bot Bot commented Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

myabc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Visual or ARIA snapshot differences found

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

changeset-bot Bot commented Jan 31, 2026 •

edited

Loading

github-actions Bot commented Feb 18, 2026 •

edited

Loading