feat: create responsive table #8079
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8079 +/- ##
==========================================
- Coverage 73.20% 73.15% -0.05%
==========================================
Files 97 97
Lines 8448 8448
Branches 227 228 +1
==========================================
- Hits 6184 6180 -4
- Misses 2263 2267 +4
Partials 1 1 ☔ View full report in Codecov by Sentry. |
avivkeller
dismissed
their stale review
I actually realized a concern I have with this. I'm retracting my approval, but I won't block.
Is there a way to make this match the syntax of a normal table, and use children rather than props? With props, we can't add this to our MDX processor, and such, MDX tables (i.e. https://nodejs-5rit8fohj-openjs.vercel.app/en/learn/getting-started/debugging#command-line-options) will not be responsive.
|
The tables here pail in comparison (size wise) to the ones in the node core, so markdown compatibility is really important to me on this |
There was a problem hiding this comment.
Pull Request Overview
This PR creates a responsive table component system that transforms traditional HTML tables into mobile-friendly layouts. The implementation includes utilities for parsing table structures, responsive desktop/mobile views, and integration with MDX for seamless table rendering.
- Adds table parsing utilities to extract columns and data from React table elements
- Creates a responsive table component that shows traditional tables on desktop and card-based layouts on mobile
- Integrates the responsive table with MDX rendering and refactors an existing table implementation
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/util/table.ts | Utility functions for parsing table structure from React elements |
| packages/ui-components/src/types.ts | Type definitions for table columns and data |
| packages/ui-components/src/MDX/Table/index.tsx | MDX table component wrapper using the responsive table |
| packages/ui-components/src/Common/ResponsiveTable/index.tsx | Main responsive table component with desktop/mobile views |
| packages/ui-components/src/Common/ResponsiveTable/index.stories.tsx | Storybook stories for the responsive table component |
| packages/ui-components/src/Common/ResponsiveTable/MobileTable/index.tsx | Mobile card-based table implementation |
| packages/ui-components/src/Common/ResponsiveTable/MobileTable/index.module.css | Styles for mobile table layout |
| packages/ui-components/src/Common/ResponsiveTable/DesktopTable/index.tsx | Desktop traditional table implementation |
| packages/ui-components/src/Common/Card/index.tsx | Card component used by mobile table |
| packages/ui-components/src/Common/Card/index.stories.tsx | Storybook stories for card component |
| packages/ui-components/src/Common/Card/index.module.css | Card component styles |
| apps/site/next.mdx.use.client.mjs | Integration of table component with MDX rendering |
| apps/site/components/Releases/PreviousReleasesTable.tsx | Refactored existing table to use responsive table component |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
I also believe this file could benefit from some cleanup, inline documentation+jsdocs and please use brackets on if statements.
| const thead = nodes.find(node => isTableElement(node, 'thead')); | ||
| const tbody = nodes.find(node => isTableElement(node, 'tbody')); | ||
|
|
||
| if (!thead) throw new Error('Thead element not found'); |
There was a problem hiding this comment.
I don't think Markdown tables require headers, or do they?
| @@ -0,0 +1,98 @@ | |||
| import { | |||
There was a problem hiding this comment.
Separate type imports from actual imports.
|
|
||
| import type { TableColumn, TableData } from '#ui/types'; | ||
|
|
||
| const hasChildren = (props: unknown): props is { children: ReactNode } => |
There was a problem hiding this comment.
| const hasChildren = (props: unknown): props is { children: ReactNode } => | |
| const hasChildren = (props: {}): props is PropsWithChildren<typeof props> => |
| <td data-label="Date"> | ||
| <FormattedTime date={release.currentStart} /> | ||
| </td> | ||
| const columns = [ |
There was a problem hiding this comment.
Here's my spicy take on this:
- I don't like this design of passing header/data as props
- This responsive table component should work as the regular HTML table
- Pretty much should just be
import Tablefrom#ui/common/Table - That common table could already come with a mapper to map the children into the proper props
- Then you won't need a MDX version of this component. I believe you can also handle the type of the component as the same type of an actual HTML table.
What is really confusing here is the separate DOM structure for Mobile and Desktop tables and that you are pretty much rendering a much larger and complex DOM structure https://github.com/nodejs/nodejs.org/pull/8079/files#diff-e77faf52f4e475e7e36e8b4f5c935b9a8c2fb506f89b70ec130a6ed602007c0cR13-R25 for every table.
I know this might sound harder, but I do really believe you can achieve all you want with pure CSS.
I don't even think we need a custom component, it's like we're reinventing the wheel here. I do believe you can achieve all you want purely based on CSS. Might be harder? Hell yes, even more that heading separation. Below is an example query I gave to ChatGPT:
Check the example styles below:
/* Base table styles (desktop) */
table {
width: 100%;
border-collapse: collapse;
}
thead th {
text-align: left;
padding: 8px;
}
tbody td {
padding: 8px;
border-bottom: 1px solid #ddd;
}
/* Mobile layout: stack each row */
@media (max-width: 600px) {
thead {
/* Hide the table header visually but keep it for accessibility */
position: absolute;
clip: rect(0 0 0 0);
width: 1px;
height: 1px;
overflow: hidden;
}
table, tbody, tr, td {
display: block;
width: 100%;
}
tr {
margin-bottom: 1em;
border: 1px solid #ccc;
padding: 8px;
}
td {
/* Indent data and make space for label */
position: relative;
padding-left: 50%;
text-align: left;
}
td::before {
content: attr(data-title);
position: absolute;
top: 50%;
left: 8px;
transform: translateY(-50%);
font-weight: bold;
white-space: nowrap;
}
}How It Works
- Desktop View: The table appears normally with headers on top.
- Mobile View (
max-width: 600px):- The
<thead>is visually hidden for space-saving, but remains accessible. - Each
<tr>becomes a block, spaced out like its own mini-card. - Each
<td>displays a label from itsdata-titleattribute using::before, mimicking column headers.
- The
There was a problem hiding this comment.
Note the above is just an example, but you can absolutely 100% achieve this with pure CSS.
There was a problem hiding this comment.
I actually don't think you can achieve the card-based layout with CSS (since, it requires addition text to be displayed) but you definitely can with a custom component matching the syntax of a table.
There was a problem hiding this comment.
You posted this twice. But yes, it is possible to do it with pure CSS.
There was a problem hiding this comment.
You posted this twice.
Oh, GitHub Mobile 😭
attr(data-title)
This would still require a custom component / modifying the markdown to add this field.
There was a problem hiding this comment.
Not really... You can do that directly on Remark.
There was a problem hiding this comment.
But if not on Remark, yes, you can make a wrapper, like a WithResponseTable that just introspects the thead and tbody and adds the title tag to the tr's. But I do prefer the remark path, as it can be easily done as a plugin for specifcially table elements, and visits the Nodes and then just updates the attributes :)
Which adds a bit more of processing (CPU) but at least less JSX/interpretation for the client-side.
Description
Creates a responsible table
Validation
Related Issues
Fixes #7656
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.