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

Cannot reassign same data instances to table when hierarchy is enabled #2265

Closed
mollykreis opened this issue Jul 12, 2024 · 0 comments · Fixed by #2300
Closed

Cannot reassign same data instances to table when hierarchy is enabled #2265

mollykreis opened this issue Jul 12, 2024 · 0 comments · Fixed by #2300
Assignees
Labels
bug Something isn't working

Comments

@mollykreis
Copy link
Contributor

🐛 Bug Report

With the nimble-table, it is expected that you can pass the same array/object instances with modified property values to setData() and have those updates be reflected in the rendered table. This works as expected when hierarchy is not enabled -- in fact, there's a test that verifies this works as expected. However, when hierarchy is enabled (specifically when parentIdFieldName is configured), this does not work. It looks like the issue is that the arrayToTree code path does not ensure a copy of each data record is made in order for TanStack to recognize the change.

💻 Repro or Code Sample

🤔 Expected Behavior

You should be able to write code like this and have the table reflect the changes:

const data= [{ a: 'hello' }];
table.setData(data);
// table renders 'hello'

data[0].a = 'world';
table.setData(data);
// table renders 'world' if parentIdFieldName is not configured
// table still renders 'hello' if parentIdFieldName is configured

😯 Current Behavior

See expected behavior

💁 Possible Solution

The arrayToTree copy code path in the table needs to ensure that a copy of the data records are made so that TanStack recognizes that there was an update to the data.

🔦 Context

🌍 Your Environment

  • OS & Device: [e.g. MacOS, iOS, Windows, Linux] on [iPhone 7, PC]
  • Browser [e.g. Microsoft Edge, Google Chrome, Apple Safari, Mozilla FireFox]
  • Version [e.g. 1.8.0]
@mollykreis mollykreis added bug Something isn't working triage New issue that needs to be reviewed labels Jul 12, 2024
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Jul 15, 2024
@mollykreis mollykreis self-assigned this Jul 23, 2024
mollykreis added a commit that referenced this issue Jul 31, 2024
# Pull Request

## 🤨 Rationale

Resolves #2265 

## 👩‍💻 Implementation

The `arrayToTree` code path for setting data in the table does not make
a copy of each record in the same way that the other `setData` code
paths do. Therefore, I updated the `arrayToTree` function to copy the
record as it turns the array into a tree.

## 🧪 Testing

- Added a new unit test that failed before my change and now passes
- Note, there already is a similar test for non-hierarchical data, so I
only added a new test for hierarchical data.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
rajsite added a commit that referenced this issue Aug 5, 2024
…ed (#2329)

# Pull Request

## 🤨 Rationale

The menu button column in the Angular example app had workarounds for
issues #2266 and #2265.
The menu button column in the Blazor example app had a workaround for
#2266.

Now that both of those issues have been fixed, I'm removing those
workarounds.

## 👩‍💻 Implementation

- Update templates in example apps to add/remove the check icon in the
color menu rather than updating its styling
- Update Angular example app to not make an additional copy of the
modified record

## 🧪 Testing

Verified that both example apps still work as expected.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

Co-authored-by: Milan Raj <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

2 participants