Skip to content

[DREAM-590] Create a new TableComponent based on Primer React's DataTable - #409

Open
bsatarnejad wants to merge 17 commits into
mainfrom
70204-create-a-new-table-component-based-on-primer-reacts-datatable
Open

[DREAM-590] Create a new TableComponent based on Primer React's DataTable#409
bsatarnejad wants to merge 17 commits into
mainfrom
70204-create-a-new-table-component-based-on-primer-reacts-datatable

Conversation

@bsatarnejad

@bsatarnejad bsatarnejad commented Jan 31, 2026

Copy link
Copy Markdown

What are you trying to accomplish?

Introduces two new components to Primer::OpenProject, ported from Primer React's DataTable:

  • Primer::OpenProject::Table — a low-level, semantic table primitive (<table> + caption/colgroup/thead/tbody/tfoot/tr/th/td) built from small slot-based subcomponents. Not intended for direct use except for advanced cases.
  • Primer::OpenProject::DataTable — a higher-level, column-config-driven component built on Table. Supports a title/subtitle, title-row actions with an optional divider (Table.Actions/Table.Divider), per-column alignment and width, row headers, custom cell renderers, muted placeholders for blank cell values (Table.CellPlaceholder), an automatic Blankslate-backed empty state, addressable rows via a row_id: proc, client-side or fully server-side (sorting: :external) sorting with a server-resolved initial sort, and an optional pagination footer (range summary + page navigation, ported from Table.Pagination).

Sorting uses a hybrid model: the server resolves and renders the initial sort order, and a Catalyst custom element (<data-table>) handles subsequent client-side sorting. The element sorts on per-cell metadata (data-sort-value / data-sort-type / data-sort-strategy) rather than rendered text, with basic, datetime, and alphanumeric (natural-sort) strategies, stable ordering, and blank-values-last — matching React's behaviour. Columns without a backing field can supply a sort_value: proc for computed sort keys.

Screenshots

Integration

No changes required to existing production code. This is almost purely additive — two new opt-in components plus their registration in primer.ts / primer.pcss. The only touch to an existing component is Primer::OpenProject::Pagination, which gains an opt-in tag: option so the pagination footer can embed it without nesting a second <nav> landmark; its default behaviour is unchanged.

List the issues that this change affects.

https://community.openproject.org/wp/DREAM-590

Risk Assessment

  • Low risk — additive only; new isolated components with no changes to existing ones, 100% test coverage on the new code, and trivially rolled back by reverting the branch. The new global JS/CSS only activate when a consumer renders the component.

What approach did you choose and why?

  • Built a generic Table primitive first, then DataTable on top. Mirrors the React structure (Table.* compound components) and keeps the semantic HTML concerns separate from the column-config/sorting concerns.
  • Sort on metadata, not rendered text. An earlier iteration sorted the DOM by parseFloat(textContent), which mis-sorted dates, currency, and numeric-in-string values ("Project 10" before "Project 2"). Cells now emit data-sort-value/data-sort-type/data-sort-strategy, so formatted cells (labels, strftime dates) sort by their underlying value.
  • Hybrid server/client sorting. The server renders the initial order so the table is correct before JS loads; the client element handles interactive re-sorts.
  • External sorting as links, not callbacks. sorting: :external leaves row order to the caller (e.g. SQL ordering): sortable headers become plain links built by a sort_href_builder proc that receives the column id and the next direction, and no client sort metadata is emitted. This deliberately diverges from React's externalSorting (sort buttons + onToggleSort JS callback) — links suit server rendering and work without JavaScript.
  • Empty state mirrors BorderBoxListComponent. An empty table automatically renders a Blankslate with an i18n title; the empty_state slot customizes title/description/icon and offers an interactive: mode that adds a polite live region — the same idiom as OpenProject's BorderBoxListComponent empty state.
  • Server-driven pagination footer. DataTable's pagination slot renders a range summary ("1 ‒ 10 of 95", with a translatable screen-reader phrasing) plus page-navigation links built from a caller-supplied href_builder — pagination state stays entirely on the server.
  • Design-token-first CSS. Uses Primer primitives (--borderRadius-medium, --text-body-size-small, etc.) rather than hardcoded values.

Anything you want to highlight for special attention from reviewers?

  • The hybrid sort model: no-JS users get the server's initial sort but cannot re-sort, and client-side sort operates on the currently-rendered rows only (no pagination awareness). Confirm this matches the intended use.
  • The list of React features not yet ported (below) — please confirm none are required for the first consumer.
  • Table is a fairly granular primitive (12 subcomponents) with a single consumer today; flagged in case a leaner surface is preferred.

Accessibility

  • No new axe scan violationaria-labelledby/aria-describedby wire the table to its title/subtitle, headers use correct scope/role (columnheader/rowheader), sortable headers expose aria-sort (ascending/descending), and unsorted sortable headers render a visually-hidden "sort ascending" hint. This matches Primer React's DataTable a11y exactly (React likewise emits no aria-sort="none" and no live-region announcement on sort).

Merge checklist

  • Added/updated tests (100% coverage on new code)
  • Added/updated documentation (YARD @param docs)
  • Added/updated previews (Lookbook: Default, With Row Actions, With Actions, With Cell Placeholder, Empty State, With External Sorting, With Pagination, With Pagination Using Default Page Index, Playground)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Not yet ported from Primer React's DataTable

Tracked for follow-up; none block the initial component:

React feature Notes
Table.Pagination aria-live status The pagination footer itself is ported (range summary + page navigation via Primer::OpenProject::Pagination); still missing is React's aria-live announcement on page change.
Table.Skeleton / loading state WithLoading. Deferred pending the async-loading approach (likely Turbo Frames). Skeleton CSS was present but had no Ruby behind it; removed in cleanup pending a real port.
Table.ErrorDialog / network-error state WithNetworkError. Deferred pending the async-loading approach.
Custom sort comparator WithCustomSorting. Columns can supply a sort_value: proc for computed sort keys, but an arbitrary per-column comparator function is still not supported.
onToggleSort sort callback WithSortEvents. No JS hook to react to client-side sort changes. Server-side sorting no longer needs it — sorting: :external covers that via links.
createColumnHelper TS typed column builder — N/A for Ruby; the with_column slot API replaces it.

Ported since this list was first written: empty/no-content state (auto Blankslate + empty_state slot), Table.Actions + Table.Divider (typed action slots + divider:), Table.CellPlaceholder (component + column placeholder:), externalSorting (sorting: :external with link headers), and getRowId (row_id: proc emitting data-row-id / optional namespaced DOM id — DOM addressing rather than React's virtual-DOM keys).

@changeset-bot

changeset-bot Bot commented Jan 31, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fb3640a

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

Comment thread app/components/primer/open_project/data_table/column.rb
@myabc
myabc self-requested a review February 3, 2026 23:48

@myabc myabc 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.

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

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
myabc requested a review from HDinger February 6, 2026 13:56
@myabc
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
Comment thread app/components/primer/open_project/table/header_row.rb
@myabc
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
@github-actions

github-actions Bot commented Feb 18, 2026

Copy link
Copy Markdown

⚠️ 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

@myabc
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
myabc requested review from Copilot February 18, 2026 01:12
@myabc
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 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread app/components/primer/open_project/data_table.html.erb Outdated
Comment thread app/components/primer/open_project/data_table.rb Outdated
Comment thread static/info_arch.json
Comment thread app/components/primer/primer.pcss Outdated
Comment thread app/components/primer/open_project/data_table.rb Outdated
Comment thread app/components/primer/open_project/data_table.html.erb Outdated
Comment thread app/components/primer/open_project/table/row_group.rb
Comment thread app/components/primer/open_project/table/row_group.rb Outdated
@myabc
myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch 2 times, most recently from 36b9f27 to f5e8f8a Compare February 18, 2026 02:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@myabc
myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch 2 times, most recently from 8655a51 to 1e40dab Compare February 18, 2026 02:31
@myabc myabc added ruby javascript work in progress Do not merge without further discussion labels Mar 8, 2026
@bsatarnejad 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
myabc requested a review from Copilot April 25, 2026 20:42
Comment thread app/components/primer/open_project/table/cell.rb Outdated
@myabc
myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch 3 times, most recently from addff3e to aa1204d Compare July 10, 2026 18:13
myabc added a commit that referenced this pull request Jul 11, 2026
Adds an explicit_roles option to the Table primitive that cascades
through Head, Body, Foot, Row, Header, and Cell via their slot
lambdas. Roles render only when the flag is set; the bare primitive
now emits clean native table markup, since explicit roles on native
table elements are redundant ARIA.

DataTable opts in, as its grid layout (display: grid on the table,
display: contents on rows) strips the native semantics that the
explicit roles restore. Its rendered output is unchanged.

Addresses PR #409 review feedback.
@myabc
myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 947cab8 to 802c5f4 Compare July 11, 2026 13:28
@myabc
myabc self-requested a review July 11, 2026 13:30
myabc added a commit that referenced this pull request Jul 21, 2026
Adds an explicit_roles option to the Table primitive that cascades
through Head, Body, Foot, Row, Header, and Cell via their slot
lambdas. Roles render only when the flag is set; the bare primitive
now emits clean native table markup, since explicit roles on native
table elements are redundant ARIA.

DataTable opts in, as its grid layout (display: grid on the table,
display: contents on rows) strips the native semantics that the
explicit roles restore. Its rendered output is unchanged.

Addresses PR #409 review feedback.
@myabc
myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 6e719a9 to 44051d5 Compare July 21, 2026 19:07
myabc added a commit that referenced this pull request Jul 21, 2026
Adds an explicit_roles option to the Table primitive that cascades
through Head, Body, Foot, Row, Header, and Cell via their slot
lambdas. Roles render only when the flag is set; the bare primitive
now emits clean native table markup, since explicit roles on native
table elements are redundant ARIA.

DataTable opts in, as its grid layout (display: grid on the table,
display: contents on rows) strips the native semantics that the
explicit roles restore. Its rendered output is unchanged.

Addresses PR #409 review feedback.
@myabc
myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 44051d5 to a6d62cc Compare July 21, 2026 19:11
bsatarnejad and others added 10 commits July 23, 2026 12:34
Adds OpenProject table primitives for building Primer-style tables with
caption, colgroup, head, body, foot, row, header, and cell slots. The
primitive components handle semantic table roles, row headers,
alignment, cell padding, column widths, and colgroup rendering.

Adds a DataTable wrapper modeled after Primer React's DataTable API. It
supports title and subtitle slots, field-backed and custom-rendered
columns, row action columns, row header cells, initial sort state, and
sortable headers with server-rendered icon state.

Implements hybrid DataTable sorting: Ruby orders the initial render,
while the Catalyst element re-sorts client-side using raw per-cell sort
values. Sortable columns use React-aligned strategies (`basic`,
`datetime`, `alphanumeric`), ASC/DESC transitions, stable ordering, and
blank values sorted last.

Adds Lookbook previews for Table and DataTable, generated static
metadata, and component/system coverage for rendering, column widths,
sorting, row actions, raw sort metadata, and preview interaction.

https://community.openproject.org/wp/DREAM-590
Adds a server-side pagination footer to DataTable, ported from Primer
React's Table.Pagination.

Introduces DataTable::PaginationFooter, exposed through a `pagination`
slot, which renders an optional range summary ("1 - 10 of 95") alongside
the existing Primer::OpenProject::Pagination. Pagination gains a `tag`
option so it can be embedded inside the footer's single nav landmark
without nesting a second nav.

Adds With Pagination and With Pagination Using Default Page Index
previews, component tests, and a system spec exercising page navigation
and the range summary.

https://community.openproject.org/wp/DREAM-590
Introduces DataTable::CellPlaceholder, a muted span ported from Primer
React's Table.CellPlaceholder, and a Column placeholder option that
substitutes it for blank cell values. Placeholder text is display-only:
blank values still sort as blank.

Custom cell renderers can also render the component directly.
Ports Primer React's Table.Actions and Table.Divider. Typed action slots
(button, icon_button, menu) render at the end of the title row via a new
'actions' grid area; an optional divider kwarg draws a presentational
rule below the title row.

Generalizes the table spacing selector since header elements may now sit
between the subtitle and the overflow wrapper.
Renders a Blankslate-backed empty state instead of the table grid when
there are no rows. A default state with an i18n title appears
automatically; the empty_state slot customizes title, description, and
icon. The API mirrors OpenProject's BorderBoxListComponent empty state,
including an interactive option that adds a polite live region for
dynamically updated tables.

Title, subtitle, and actions remain visible above the empty state,
unlike Primer React where consumers replace the whole container.
Adds a row_id proc that emits a data-row-id attribute on each
body row, plus an opt-in row_dom_id flag that also assigns a DOM
id namespaced by the table id, keeping ids unique across tables.
Rows whose proc returns a blank value get no attributes.

Diverges from Primer React's getRowId, which only feeds virtual
DOM keys: here the identifier addresses rows in the rendered DOM,
e.g. for Turbo Stream targets or test selectors.
Adds sorting: :external, leaving row order to the caller (e.g. SQL
ordering). Sortable headers render plain links built by a required
sort_href_builder proc, which receives the column id and the direction
the link requests next (NONE and DESC cycle to ASC, ASC to DESC).
aria-sort renders exactly as in client mode.

External tables emit no client sort metadata: no data-sort-strategy on
headers, no per-cell sort values, and no toggleSort binding. The
data-table element additionally ignores connect and toggleSort when the
data-external-sorting attribute is present.

Diverges from Primer React, whose externalSorting keeps sort buttons and
an onToggleSort callback; links suit server rendering and work without
JavaScript.
Adds an explicit_roles option to the Table primitive that cascades
through Head, Body, Foot, Row, Header, and Cell via their slot
lambdas. Roles render only when the flag is set; the bare primitive
now emits clean native table markup, since explicit roles on native
table elements are redundant ARIA.

DataTable opts in, as its grid layout (display: grid on the table,
display: contents on rows) strips the native semantics that the
explicit roles restore. Its rendered output is unchanged.

Addresses PR #409 review feedback.
@myabc
myabc force-pushed the 70204-create-a-new-table-component-based-on-primer-reacts-datatable branch from 7052d20 to d9c672b Compare July 23, 2026 10:34
myabc and others added 3 commits July 23, 2026 10:50
Axe flagged a heading-order violation on the empty_state preview: the
table title renders as an h2 while the Blankslate heading was hard-coded
to h4, skipping a level.

Defaults the empty-state heading to h3 and exposes a heading_tag
parameter on the slot so callers embedding the table under deeper
heading structures can adjust the level.

@HDinger HDinger 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.

@bsatarnejad @myabc I finally managed to have a first look at this. Although the component looks very nice in the lookbook, I decided to make a stop now, as I see some fundamental things which we should talk about before we continue here:

  • The components API: The current approach makes it really hard to migrate our exisiting tables to the new component. I am aware that this was written to be close to how the upstream component was written but I guess we have to talk about how strict we want to be here given that our reqirements are different from what the React component offers. Maybe the idea to re-implement the React component was wrong after all, and we should rather invest in the table component we already have in core. I am not sure any more..
  • Naming/consistency: Speaking of staying close to the upstream component: Some param names have been changed without apparent reason and there are a lot more params added. I don't know whether all of them are needed or what they are even doing as they are lacking examples in the preview. Some of the params got renamed in the middle of the component stack making it really hard to follow (e.g the procs adding proc to the name, or sorty_by being exchanged with sorting_strategy)
  • There is this second Table component which I admit did not understand why it was needed. Codewise it makes it even harder to follow.. E.g. We now have three different classes with col in the name all doing something else ? We have pagianation, footer and pagination_footer?

Let's maybe align next week and see how we can progress.

TITLE_TAG_DEFAULT = :h2

SUBTITLE_TAG_DEFAULT = :div
SUBTITLE_TAG_OPTIONS = %i[div p span].freeze

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.

Suggested change
SUBTITLE_TAG_OPTIONS = %i[div p span].freeze
SUBTITLE_TAG_OPTIONS = [SUBTITLE_TAG_DEFAULT, :p, :span].freeze

cell_padding: CELL_PADDING_DEFAULT,
initial_sort_column: nil,
initial_sort_direction: nil,
sorting: SORTING_DEFAULT,

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.

In the React version, this is a Boolean called externalSorting.

Comment on lines +149 to +153
sort_href_builder: nil,
divider: false,
row_id: nil,
row_dom_id: false,
html_data: {},

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.

All of those are not present in the React version and I see them problematic because of various reasons

  • divider should not be configurable
  • what is row_id compared to row_dom_id? Where is the difference? It is also weird to set one id for all rows which violates the uniqness. Further, row_id gets assigned to row_id_proc and is handled as such so it is not a string but a proc?
  • html_data is a strange name. Maybe table_arguments would be better?

Comment on lines +168 to +169
@container_arguments = {}
@container_arguments[:classes] = "TableContainer"

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.

Suggested change
@container_arguments = {}
@container_arguments[:classes] = "TableContainer"
@container_arguments = { classes: "TableContainer" }

Comment on lines +309 to +349
def grid_template_from_columns(columns)
columns.map do |column|
column_width = column.width || :grow
min_width = :auto
max_width = "1fr"

if column_width == :auto
max_width = :auto
end

# Setting a min-width of 'max-content' ensures that the column will grow to fit the widest cell's content.
# However, If the column has a max width, we can't set the min width to `max-content` because
# the widest cell's content might overflow the container.
if column_width == :grow && column.max_width.blank?
min_width = :"max-content"
end

# Column widths set to "growCollapse" don't need a min width unless one is explicitly provided.
if column_width == :grow_collapse
min_width = "0"
end

# If a consumer passes `min_width` or `max_width`, we need to override whatever we set above.
if column.min_width
min_width = column.min_width.is_a?(Numeric) ? "#{column.min_width}px" : column.min_width
end

if column.max_width
max_width = column.max_width.is_a?(Numeric) ? "#{column.max_width}px" : column.max_width
end

# If a consumer is passing one of the shorthand widths or doesn't pass a width at all, we use the
# min and max width calculated above to create a minmax() column template value.
if !column_width.is_a?(Numeric) && column_width.in?(%i[grow grow_collapse auto])
next min_width == max_width ? min_width : "minmax(#{min_width}, #{max_width})"
end

# If we reach this point, the consumer is passing an explicit width value.
column_width.is_a?(Numeric) ? "#{column_width}px" : column_width
end
end

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.

This method is huuuge.. Also it is not part of the previews, so I don't know whether it is actually working..

Comment on lines +130 to +132
def self.blank_value?(value)
value.nil? || value == ""
end

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.

Claude finding 🤖

blank_value? intentionally diverges from Rails' .blank? — document it
This is not a bug, but the choice to avoid Rails' .blank? is subtle and undocumented. .blank? would treat 0, false, and " " as blank — all of which are valid sort values here. A future contributor unfamiliar with this decision might "clean up" this method by substituting .blank?, silently breaking sort order for zero and boolean columns.

Add a comment explaining the intent:

# Intentionally narrow: only nil and empty string are treated as blank.
# Do NOT replace with .blank? — it would incorrectly treat 0, false, and
# whitespace-only strings as blank sort values.
def self.blank_value?(value)
  value.nil? || value == ""
end

Comment on lines +65 to +73
def self.metadata_for(value, strategy)
normalized_value = normalize_value(value, strategy)

{
blank: blank_value?(value),
type: normalized_value.is_a?(Numeric) ? "number" : "text",
value: normalized_value.to_s
}
end

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.

Claude finding 🤖

metadata_for emits wrong data-sort-type for numeric values in :alphanumeric columns

For :alphanumeric columns, normalize_value returns the raw value unchanged. If that value is a Numeric (e.g. an integer ID), type is set to "number". On the JS side, getSortValue
then returns a Number, which gets passed into alphanumeric(String(valueA), String(valueB)) — producing correct results only by accident, since String() silently coerces the number back.

The real problem is that type is used by the JS client to decode the sort value, not to select the sort strategy. For :alphanumeric, the type should always be "text" regardless of the Ruby value's class:

  def self.metadata_for(value, strategy)
    normalized_value = normalize_value(value, strategy)
    {
      blank: blank_value?(value),
      type: (strategy != :alphanumeric && normalized_value.is_a?(Numeric)) ? "number" : "text",
      value: normalized_value.to_s
    }
  end

.TablePreview [data-cell-align="end"] {
text-align: end;
}
</style>

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.

Why are there styles in the preview to add something that is not part of the component?

# @param show_colgroup toggle
# @param show_footer toggle
# @param controller text
def playground(

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.

Is that really a component we want to communicate to the outside? Or only for internal use?

# @param initial_sort_direction [Symbol] select [none, ASC, DESC]
# @param rows_count [Integer] number
# @param show_subtitle toggle
def playground(

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.

Please move the playground up to be below default

myabc and others added 4 commits July 29, 2026 12:29
Downstream tables carry per-column CSS classes and data attributes
that vary by row, and today the only way to reach a cell's element is
to take over rendering the cell entirely.
Downstream tables mark rows with state-derived classes and data
attributes, which row_id alone cannot express.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor release work in progress Do not merge without further discussion

Development

Successfully merging this pull request may close these issues.

5 participants