skeleton icon indicating copy to clipboard operation
skeleton copied to clipboard

Bugfix/recursivetreeview issue after changing nodes

Open KaiStarkk opened this issue 1 year ago • 8 comments

Draft

Description

This adds tests for a bug identified in #2875 . This adds 4 new tests for RecursiveTreeView which isolates the issue to one specific test, the other 3 pass.

I've submitted this PR as draft since the tests are useful, but this isn't yet a fix. Will work on it shortly.

Checklist

Please read and apply all contribution requirements.

  • [x] This PR targets the dev branch (NEVER master)
  • [x] Documentation reflects all relevant changes
  • [x] Branch is prefixed with: docs/, feat/, chore/, bugfix/
  • [x] Ensure Svelte and Typescript linting is current - run pnpm ci:check
  • [x] Ensure Prettier linting is current - run pnpm format ^ This caused an unrelated edit which has been added as a separate commit
  • [ ] All test cases are passing - run pnpm test ^ There were already failing tests on dev
  • [x] Includes a changeset (if relevant; see above)

KaiStarkk avatar Oct 14 '24 13:10 KaiStarkk

🦋 Changeset detected

Latest commit: 4fff284b9427b48867eac394d04d0b5effcf7827

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@skeletonlabs/skeleton Patch

Not sure what this means? Click here to learn what changesets are.

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

changeset-bot[bot] avatar Oct 14 '24 13:10 changeset-bot[bot]

@KaiStarkk is attempting to deploy a commit to the Skeleton Labs Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Oct 14 '24 13:10 vercel[bot]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
skeleton-docs ✅ Ready (Inspect) Visit Preview Oct 14, 2024 1:49pm

vercel[bot] avatar Oct 14 '24 13:10 vercel[bot]

Just a note regarding the package upgrades, only the upgrades to testing packages were strictly necessary (vite-plugin-svelte and jest-dom) to get tests working properly, but I've aimed to resolve all updates to this point in time.

KaiStarkk avatar Oct 14 '24 14:10 KaiStarkk

Than you @KaiStarkk, please make sure to mark the PR "ready for review" when it reaches that state. We'll review and aim to provide feedback and/or merge asap.

endigo9740 avatar Oct 14 '24 17:10 endigo9740

@KaiStarkk another quick reminder this is awaiting your final changes and the status being bumped up to "ready for review". Please let me know if this is continuing forward, otherwise I'll assume this is abandoned and remove. Please and thanks!

endigo9740 avatar Oct 21 '24 18:10 endigo9740

Hi @endigo9740 , I had another look on the weekend but wasn't able to figure out the problem. Assuming the tests are still valuable, should I switch this to ready and rename it as adding the tests, rather than a fix?

I will have another go at it this weekend too

KaiStarkk avatar Oct 22 '24 00:10 KaiStarkk

@KaiStarkk I would think if the tests are failing then this is not in a functional state. Unfortunately I am just completely absorbed in our v3 progress right now, so I can't really pause to help at the moment. But I'll bring this up with the team and see if they have any other ideas.

endigo9740 avatar Oct 22 '24 17:10 endigo9740

These tests are still valid but anyone searching can cherry pick if needed. Closing as Svelte 5 is out now, I see that it makes much more sense to look into Skeleton 3

KaiStarkk avatar Oct 27 '24 13:10 KaiStarkk