Skip to content

Conversation

ericsoderberghp
Copy link
Contributor

What does this PR do?

Changed DataTable to use a primaryKey value as the row key

Where should the reviewer start?

one line changed

What testing has been done on this PR?

unit tests

How should this be manually tested?

storybook?

Do Jest tests follow these best practices?

no change to test structure

Any background context you want to provide?

What are the relevant issues?

#6688

Screenshots (if appropriate)

Do the grommet docs need to be updated?

no

Should this PR be mentioned in the release notes?

yes

Is this change backwards compatible or is it a breaking change?

backwards compatible when using primaryKey
if the caller has no column.primary or primaryKey and the first value in the data objects is not unique, the browser will display a warning. This is expected given the inputs.

@ericsoderberghp ericsoderberghp linked an issue Apr 11, 2023 that may be closed by this pull request
Copy link
Contributor

@JCoder121 JCoder121 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 to me!

@taysea
Copy link
Collaborator

taysea commented Apr 12, 2023

Screen Shot 2023-04-12 at 9 38 52 AM

Something funky is happening in this storybook example. The location is showing up in the "Name" column

@JCoder121
Copy link
Contributor

JCoder121 commented Apr 12, 2023

Screen Shot 2023-04-12 at 9 38 52 AM

Something funky is happening in this storybook example. The location is showing up in the "Name" column

Screen Shot 2023-04-12 at 9 38 52 AM

Something funky is happening in this storybook example. The location is showing up in the "Name" column

I reproduced by interacting with the storybook example on Chrome. Seemed to only be an issue when interacting with DataTableColumns -> selecting/deselecting the Name column.

@ericsoderberghp
Copy link
Contributor Author

The story issue was caused by the DataTableColumns Simple story not setting primaryKey. This is needed because the primary flag set on the name column in columns is lost when the name column is removed. We need to set the primaryKey to allow the column it refers to to be removed but still used as the key from the data.

Copy link
Collaborator

@taysea taysea 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!

Copy link
Collaborator

@jcfilben jcfilben 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!

@ericsoderberghp ericsoderberghp merged commit a7d6555 into master Apr 13, 2023
@ericsoderberghp ericsoderberghp deleted the fix/datatable-row-key branch April 13, 2023 19:18
@jkalberer
Copy link

Thanks for doing this.

Will there be a grommet release in the next couple of weeks that includes this fix?

@jcfilben
Copy link
Collaborator

Will there be a grommet release in the next couple of weeks that includes this fix?

We should have a release coming roughly within ~1 week

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.

DataTable ignoring primaryKey prop
5 participants