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: use index for deleting asset [ZEND-5025] #7932

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

whitelisab
Copy link
Contributor

Purpose

This PR to get the dam-app-base ready to support React 18 introduced some changes to the deleteItem login in the SortableComponent. These changes caused issues with deleting, where when clicking on an item in the list to delete, the first item instead got deleted.

Approach

The key was being used to splice the item from the list, but this turned out to be the URL of the asset, so this was getting passed as NaN and then removing the first item in the list. This PR updates the deleteItem logic to instead use index instead of key.

Testing steps

I tested this locally using npm pack to test the changes to the dam-app-base within the Bynder app.

Breaking Changes

Dependencies and/or References

Deployment

After updating the dam-app-base we will open a PR for the Bynder app to bump the package version.

@whitelisab whitelisab requested a review from a team as a code owner June 3, 2024 13:42
Copy link

netlify bot commented Jun 3, 2024

Deploy Preview for ecommerce-app-base-components canceled.

Name Link
🔨 Latest commit 9133fa8
🔍 Latest deploy log https://app.netlify.com/sites/ecommerce-app-base-components/deploys/665dc85d0cce8c0008fd6467

@ethan-ozelius-contentful
Copy link
Contributor

Looks solid, really good find. Sorry about introducing this bug when upgrading from react 16 || 17 - 18. Obviously non-blocking since this bug in currently in production and should be fixed asap, but this functionality feels like something that can be unit-tested pretty easily. Might be worth a 1-pointer task to follow up someday.

@whitelisab whitelisab merged commit af76064 into master Jun 3, 2024
12 checks passed
@whitelisab whitelisab deleted the fix/dam-app-base-delete branch June 3, 2024 14:58
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