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

fix: Passing ID to "Actions" column (docs) #297

Closed
wants to merge 4 commits into from

Conversation

jeffpohlmeyer
Copy link

Issue

An undefined value was being passed in as props to the "Actions" column in the "Update columns definition" section. This is because the accessor was only passing in the email value so the cell method had no knowledge of the ID of the associated item. Also, the ID value lives on the item.value not the item.

## Issue
An `undefined` value  was being passed in as props to the "Actions" column in the "Update columns definition" section. This is because the `accessor` was _only_ passing in the email value so the `cell` method had no knowledge of the ID of the associated item. Also, the ID value lives on the `item.value` not the `item`.
@changeset-bot
Copy link

changeset-bot bot commented Sep 15, 2023

⚠️ No Changeset found

Latest commit: 41c0f6b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Sep 15, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @huntabyte on Vercel.

@huntabyte first needs to authorize it.

@jeffpohlmeyer
Copy link
Author

FWIW @huntabyte I did a little screen recording showing the effect with the current implementation in the docs and my proposed change. I was at first just recreating the tables in the docs (what would be really cool would be REPLs associated with each item in the docs but that would be a hell of a lot of work) to try to get a feel for how Data Tables work.

https://youtu.be/iOn79iu8uHE

@jeffpohlmeyer jeffpohlmeyer changed the title fix: Passing ID to "Actions" column fix: Passing ID to "Actions" column (docs) Sep 15, 2023
@huntabyte
Copy link
Owner

Hey @jeffpohlmeyer thanks for this! Something I recently discovered is that I'm doing things incorrectly in the data table, specifically for the select/checkbox column, as well as the actions column. Those columns should be using table.display() instead of table.column (see here).

If you're up for refactoring the table to use that (which prevents some weird bugs that can occur when hiding/filtering) then feel free! Otherwise I will update it soon as I have some time!

@jeffpohlmeyer
Copy link
Author

Sounds good. I'll definitely give it a shot but probably won't have a chance before Monday.

@jeffpohlmeyer
Copy link
Author

Hey @jeffpohlmeyer thanks for this! Something I recently discovered is that I'm doing things incorrectly in the data table, specifically for the select/checkbox column, as well as the actions column. Those columns should be using table.display() instead of table.column (see here).

If you're up for refactoring the table to use that (which prevents some weird bugs that can occur when hiding/filtering) then feel free! Otherwise I will update it soon as I have some time!

I'm not sure that the actions column should be using table.display() instead of table.column() because you actually need the id value to be passed into the data-table-actions.svelte component and https://svelte-headless-table.bryanmylee.com/docs/api/create-columns#table-display-displaydef-displaycolumn indicates that you use display for "non data-related information to be displayed"

@huntabyte
Copy link
Owner

I'm not sure that the actions column should be using table.display() instead of table.column() because you actually need the id value to be passed into the data-table-actions.svelte component and svelte-headless-table.bryanmylee.com/docs/api/create-columns#table-display-displaydef-displaycolumn indicates that you use display for "non data-related information to be displayed"

@jeffpohlmeyer I thought about that as well, but the way I interpreted that is this represents something specific/unique within the dataset. The issues start to become more obvious when you also want to display an id column for example, especially with filtering, other plugins, etc.

We can still destructure and access the id of the specific row it's being rendered for to pass as a prop to the component, but we aren't necessarily displaying it as a part of the data if that makes sense lol

@jeffpohlmeyer
Copy link
Author

The only issue I ran into, though, when using the display method instead of the column method was accessing the data. The library assigns its own ID value to each entry and the data exists in the row.original.id element, but when I tried using that I got a funky type error.

I'll play around a bit more in the morning (Eastern time)

@huntabyte
Copy link
Owner

huntabyte commented Sep 18, 2023

No worries @jeffpohlmeyer, I recently did it and was trying to find the branch but I guess I never committed it.

To get around that type of error, you have to narrow the typing using this function here.

So something like:

cell: ({ row }) => {
	if (row.isDisplay()) {
		// now I know the type and can access row.original
		console.log(row.original.id)
	}
}

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