Skip to content

feat: cmd-111 support no header demo in mobile table#323

Merged
wesleyboar merged 9 commits intofeat/cmd-111-new-mobile-table-uifrom
feat/cmd-111-new-mobile-table-ui--support-no-header-demo
Mar 19, 2024
Merged

feat: cmd-111 support no header demo in mobile table#323
wesleyboar merged 9 commits intofeat/cmd-111-new-mobile-table-uifrom
feat/cmd-111-new-mobile-table-ui--support-no-header-demo

Conversation

@wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Mar 18, 2024

Overview

Support the "No Header" table in a responsive table at a narrow screen width. I.e.

Fix "No Header" Not Being Responsive But Prevent Space for Missing Col. Headers
fix avoid

Related

Changes

  • changed getColFromRow to dataColAttr; it now conditionally prints attribute too, not just value
  • refactored table:has([data-col]) to :--table-has-data-col, so the selectors can be edited in one place
  • changed basic table mobile styles to "make each cell behave like a 'row'" only if the table has a <thead>
  • added selector table:not(:has(thead)) to mobile table UI (via custom selector :--table-has-data-col)
  • removed cruft class has-data-cols

Testing

  1. Open "No Header" table on narrow screen.
  2. Verify it has small-screen UI, but without excess space for missing column headers.
  3. Open "No Header" table on wide screen.
  4. Verify it has not changed appearance.
  5. Open other tables (specifically Basic and Nested) on wide and narrow screen.
  6. Verify they are unchanged.

UI

1 & 2 — "No Header" on Narrow Screen

CMS Docs
no-header narrow no-header narrow cms no-header narrow docs

3 & 4 "No Header" on Wide Screen

CMS Docs
no-header wide no-header wide cms no-header wide docs

5 & 6 — "Basic" & "Nested" on Wide & Narrow Screen

Wide Narrow
Nested nested wide nested narrow
Basic basic wide basic narrow

@github-actions github-actions bot added the feature A new feature or replacement of existing feature label Mar 18, 2024
@wesleyboar wesleyboar added the patch A backward-compatible fix label Mar 18, 2024
@wesleyboar wesleyboar marked this pull request as ready for review March 18, 2024 15:20
Copy link
Contributor

@R-Tomas-Gonzalez R-Tomas-Gonzalez left a comment

Choose a reason for hiding this comment

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

@wesleyboar
Copy link
Member Author

wesleyboar commented Mar 19, 2024

@R-Tomas-Gonzalez, our internal discussion about an even cleaner way to write this led to a rabbit hole that is CMD-84. I fear any clean-up of the code relevant to this change will lead to me starting that task prematurely.

How about you evaluate whether this bug fix both works and is a general improvement, then I'll tackle CMD-84 (in a branch off of feat/cmd-111-new-mobile-table-ui) to make our table code more easy to follow.

Details
  • I considered renaming :--table-has-data-col to :--table--is-responsive and making :--table--is-responsive include .s-paragraph-table and .paragraph-table, but not all :--table--is-responsive code is relevant to .s-paragraph-table.
  • I changed :--s-paragraph-table to wrap .s-paragraph-table and .paragraph-table in :is(…) to reduce rendered CSS complexity.

When I attempted to go further, I was challenged by the inconsistent overlap between "paragraph" table and "has data col" tables, and the undoing of borders on small-screen that should instead be adding the borders only for wide screen.

At that point, I could tell I would be balancing too much refactor in my head for a missing demo feature.

@wesleyboar
Copy link
Member Author

Renamed :--table-has-data-col to :--table--is-responsive (because it now includes table:not(:has(thead))).

Copy link
Contributor

@R-Tomas-Gonzalez R-Tomas-Gonzalez left a comment

Choose a reason for hiding this comment

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

Looks good. Intention is to build off of this in the future to clean it up.

@wesleyboar
Copy link
Member Author

Yes, via CMD-84.

@wesleyboar wesleyboar merged commit 9093435 into feat/cmd-111-new-mobile-table-ui Mar 19, 2024
@wesleyboar wesleyboar deleted the feat/cmd-111-new-mobile-table-ui--support-no-header-demo branch March 19, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature A new feature or replacement of existing feature patch A backward-compatible fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants