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

feat(data-table): pass row to display function #1810

Conversation

Pierstoval
Copy link
Contributor

This will allow display usage to use the whole object for use with computed values, or when needing multiple fields to show more than one element (like "item actions" that might need more than one field, for instance).

Copy link
Collaborator

@metonym metonym left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time – I think this is a valuable change.

The docs/types are auto-generated.

Can you update the JSDoc types in DataTable.svelte (Lines 5, 6, 10) and run yarn build:docs?

@Pierstoval Pierstoval force-pushed the datatables-inject-row-in-cells branch from 4415117 to 54d8278 Compare September 28, 2023 13:56
@Pierstoval
Copy link
Contributor Author

Can you update the JSDoc types in DataTable.svelte (Lines 5, 6, 10) and run yarn build:docs?

According to the docs, only line 10 needs update: the "display" method receives the row object only for cells, not headers, am I right?

Just rebased & pushed an update

@metonym
Copy link
Collaborator

metonym commented Sep 28, 2023

I believe that the display is reflected in the DataTableNonEmptyHeader and DataTableEmptyHeader interfaces.

When testing the types, I see this:

Screenshot 2023-09-28 at 8 53 59 AM

Repro: try using row in the type test for DataTable:

{ key: 'cost', value: 'Cost', display: (cost) => cost + '' },

@Pierstoval Pierstoval force-pushed the datatables-inject-row-in-cells branch from 54d8278 to 2966387 Compare September 30, 2023 10:06
@Pierstoval
Copy link
Contributor Author

I don't see a reason to make this change on headers: row object should not be accessible when displaying headers 🤔

This means two things:

  • The row object should be nullable for headers, and non-null for rows, I don't know how to do that
  • The display function is overriden from the headers object, that's probably the cause of these typing issues:
    rows[row.id] = headerKeys.map((key, index) => ({
    key,
    value: resolvePath(row, key),
    display: headers[index].display,
    }));

I think we should change the behavior for that.

I just pushed a fix to add row?: ... to header-related display functions, and kept row: ... for cell-related.

I'm not sure the rest of the PR is okay, feel free to make more comments 👌

@Pierstoval Pierstoval force-pushed the datatables-inject-row-in-cells branch from 2966387 to 07852fc Compare October 1, 2023 10:14
@metonym metonym changed the title DataTable: Inject "row" variable as 2nd argument in "display" function for cell items feat(data-table): pass row to display function Oct 1, 2023
@metonym metonym merged commit 9456eaa into carbon-design-system:master Oct 1, 2023
2 checks passed
@Pierstoval Pierstoval deleted the datatables-inject-row-in-cells branch October 2, 2023 07:23
@Pierstoval
Copy link
Contributor Author

Thank you @metonym !

@metonym
Copy link
Collaborator

metonym commented Oct 13, 2023

Released in v0.81.0

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.

2 participants