Conversation
|
I believe this impacts #268 but does not close it |
|
We decided for this implementation to always render buttons for the sort action even though sorting is often handled with a URL change and full page (or at least table) reload (which usually requires anchors). This has been done because the sorting action is not a navigation to a new resource per se, but rather a change in the way the same data is displayed. |
c952a0f to
be70225
Compare
|
@Mintoo200 Thanks for this. The three-state cycle and the controlled/uncontrolled split are the right shape, and the story documenting the sparse-array behaviour is welcome. I checked out Blocking
Controlled mode ignores the Sort button has no accessible name and no To discussThe DSFR bump does not belong in this PR. It changes Half-applied 1.15 structure. DSFR 1.15 wants Positional OptionalRaw class strings ( Heads up: #340 rewrites the same component (size, header column, cell alignment) and #268 tracks the whole update. Worth agreeing on a single track. Nothing to flag on security. |
| <button | ||
| className={cx( | ||
| `fr-btn--sort`, | ||
| order === "ascending" && "fr-btn--sort-asc", | ||
| order === "descending" && "fr-btn--sort-desc" | ||
| )} | ||
| onClick={() => { | ||
| onSort(); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Three divergences from the DSFR reference markup (example/component/table/index.html), which emits <button type="button" class="fr-btn--sort fr-btn fr-btn--sm">Trier</button>:
- No
type. A<button>inside a<form>defaults totype="submit", so clicking a sort control submits the form. - No
fr-btn fr-btn--sm..fr-btncarriesdisplay: inline-flexandalign-items: center; without it the::beforearrow icon falls back to default button rendering. fr-btn--sortandfr-btn--sort-ascare emitted together. The reference uses one or the other..fr-btn--sort::before(arrow-up-down) and.fr-btn--sort-asc::before(arrow-up) have equal specificity, so the right icon only wins by source order indsfr.css.
| <button | |
| className={cx( | |
| `fr-btn--sort`, | |
| order === "ascending" && "fr-btn--sort-asc", | |
| order === "descending" && "fr-btn--sort-desc" | |
| )} | |
| onClick={() => { | |
| onSort(); | |
| }} | |
| /> | |
| <button | |
| type="button" | |
| className={cx( | |
| fr.cx("fr-btn", "fr-btn--sm"), | |
| order === "ascending" | |
| ? "fr-btn--sort-asc" | |
| : order === "descending" | |
| ? "fr-btn--sort-desc" | |
| : "fr-btn--sort" | |
| )} | |
| onClick={onSort} | |
| /> |
What the suggestion cannot carry: the button still has no accessible name. The reference puts visible text inside it (clipped by max-width: 2rem; overflow: hidden), which is what screen readers and voice control announce. Table has no i18n yet — Pagination shows the createComponentI18nApi pattern to follow for a translatable "Trier".
There was a problem hiding this comment.
Applied in #a45d5280 and #cb036c42. I did not use the ternary as I find simple chained && easier to read, but I can change that if you prefer the ternary form
50ea333 to
cb036c4
Compare
|
Hey @kevbarns , thanks for the quick review.
I openned a new PR (#517) with only this bump. To note, this PR (sortable columns) depends on that bump, as 1.14 has a bug where several adjacent sortable columns would display on top of each other instead of next to each other.
In this new PR, I added
Added to #517
Do you want me to add that to the dependency update PR or to a separate "feature" PR ?
✅ Fixed in
✅ Fixed in
The update has been moved to #517 with all related issues (except the new variants as discussed above). As stated above, this PR needs at least v1.14.3 which fixes a bug on the sort button. I rebased this pull request on #517 for it to work, but it will need to cascade back on main once #517 is merged and it will still display changes from #517 until then.
✅ applied in
✅ fixed in
✅ fixed in
The changes added by #340 do add some elements provided in this pull request, such as the structure of the component, but not the sortable columns feature. We are currently developing a table-heavy set of applications and we expect to open several update on the table component in the coming months. Since #340 has not had any update in a couple of years, and given the quantity and impact of its changes, could we consider merging these requests as they come and update that pull request to remove features as they are integrated ? |
|
Second pass, @Mintoo200. Verified on Settled, checked against the branch code and not the commit names:
Blocking
To discuss1.14.3 may be all this PR needs, which would unblock it from #517. The only sort change between 1.14.2 and 1.14.3 in 1.14.3 already ships
Optional
The reference wraps the header label in On #340: merging as features come and trimming #340 in step seems right to me, but that call is @garronej's and @revolunet's, not mine. I asked for the arbitration there. Nothing on security. |
cb036c4 to
5d2f07f
Compare
To avoid desynchronisation between headers and sortable columns, they are now defined in the same place
✅ fixed in #517 and rebased
✅ fixed (?) in #517 and rebased
I rebased onto main and updated
✅ Moved to #518
👌
cf #517
This is not documented explicitly in the code documentation for the Table component. Should I add them anyway ?
👌 |
5d2f07f to
4eda7bc
Compare
|
Hey, any update on this ? |
|
Hey, it's been 3 weeks, can we merge this one ? @kevbarns @revolunet @garronej |
|
Hello @Mintoo200, Thank you for the contribution, and apologies for the delayed review. I have a broader concern about extending The DSFR table documentation describes the expected UX and visual states for sortable columns, but Building and maintaining a fully featured data table is a substantial undertaking: beyond sorting, it quickly raises questions around filtering, pagination, server-side loading, selection, accessibility, keyboard interactions, virtualisation, and API design. For projects that need such functionality, my recommendation has generally been MUI X Data Grid. Within a There is also an architectural concern with this PR: adding local sort state ( The existing That said, I would be open to a separate Best, |
|
Hi @garronej, Thank you for your feedback. Using MUI Data Grids is not an option for us for several reasons (mainly that it adds one more dependency and 39 sub-dependencies, and above all that it does not implement the table pattern, but the grid pattern which requires certain affordances that are not applicable to our, and I suspect most projects). As it stands, our best option would be to copy-paste the content of this pull request directly into our own codebase and try to keep it updated manually, which is far from ideal. I have nonetheless pushed a new commit splitting the "base" server-renderable table and the "interactive" client-side-only table. If, in the end, you would rather keep the base table, where could we document this decision to avoid a repeat of this or #340? |
9de689a to
7bf0d41
Compare
Implements sortable columns as described in the DSFR documentation for the Table component.
The technical documentation for this feature is quite sparse, but an example of its implementation is nonetheless available (if a bit hidden in the "Miscellaneous Table Story").