Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOP-5110: Render sub rows for ListTable #1342

Closed
wants to merge 8 commits into from
Closed

Conversation

rayangler
Copy link
Collaborator

@rayangler rayangler commented Jan 31, 2025

Stories/Links:

DOP-5110
Related parser PR
Sample rST page

Current Behavior:

Older tables test page

Staging Links:

Tables test page - The 2 sections pertaining to nested tables should have examples of the sub rows. All other sections can be used as controls to help identify visual regressions.

Notes:

  • Adds support for subRows in the row data and renders them in an expanded state by default.

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

Copy link

netlify bot commented Jan 31, 2025

Deploy Preview for docs-frontend-stg ready!

Name Link
🔨 Latest commit 47c0714
🔍 Latest deploy log https://app.netlify.com/sites/docs-frontend-stg/deploys/67a25c25bfa8eb00086ccfa8
😎 Deploy Preview https://deploy-preview-1342--docs-frontend-stg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 31, 2025

Deploy Preview for docs-frontend-dotcomstg ready!

Name Link
🔨 Latest commit 47c0714
🔍 Latest deploy log https://app.netlify.com/sites/docs-frontend-dotcomstg/deploys/67a25c25f9c18800089f12ca
😎 Deploy Preview https://deploy-preview-1342--docs-frontend-dotcomstg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

<Cell key={cell.id} className={cx(baseCellStyle, bodyCellStyle, isStub && stubCellStyle)} role={role}>
{cell.renderValue()}
<Cell key={cell.id} className={cx(baseCellStyle, isStub && stubCellStyle)} role={role}>
<div className={cx(bodyCellStyle)}>{cell.renderValue()}</div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to wrap cell content within a new div to ensure cell content was contained and styled separately. Whenever a row has sub rows, the "expand row" button is added as the first child of the Cell and the previous styling would cause layout issues

@rayangler rayangler marked this pull request as ready for review February 3, 2025 16:47
@rayangler rayangler requested review from seungpark and mmeigs February 3, 2025 16:54
Copy link
Collaborator

@seungpark seungpark left a comment

Choose a reason for hiding this comment

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

this looks great! thanks for working on this. minor comment below on keeping inputs close as possible !

const generateRowsData = (bodyRowNodes, columns, isNested = false) => {
// The shape of list-table's "rows" are slightly different from a traditional row directive, so we need to
// account for that
const rowNodes = isNested ? bodyRowNodes : bodyRowNodes.map((node) => node?.children[0]?.children ?? []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we handle this in where we call generateRowsData instead of handling two different types of input? i think we can call generateRowsData(bodyRows.map(...), columns) in line 287

Copy link
Collaborator

Choose a reason for hiding this comment

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

the children lookup will still have to exist (224-231) and it would be an improvement if we had the uniform structure vs (current mix of list+listitem and row), but the above comment can at least reduce the difference

Copy link
Collaborator

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

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

Looks really great

@@ -32,17 +32,24 @@ const styleTable = ({ customAlign, customWidth }) => css`
${customAlign && `text-align: ${align(customAlign)}`};
${customWidth && `width: ${customWidth}`};
margin: ${theme.size.medium} 0;

tbody[data-expanded='true'] {
// Avoid flash of light mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

@rayangler
Copy link
Collaborator Author

Closing this PR due to discussions leaning towards a no-op on our end. Please refer to the ticket comments for details.

@rayangler rayangler closed this Feb 11, 2025
@rayangler rayangler deleted the DOP-5110-nested-rows branch February 11, 2025 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants