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 #2932: check parent is null for swap delete #2933

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

jackielii
Copy link
Contributor

@jackielii jackielii commented Sep 25, 2024

Description

Please describe what changes you made, and why you feel they are necessary. Make sure to include
code examples, where applicable.

Corresponding issue: fix #2932

Testing

Please explain how you tested this change manually, and, if applicable, what new tests you added. If
you're making a change to just the website, you can omit this section.

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

@Telroshan
Copy link
Collaborator

Telroshan commented Sep 26, 2024

Hey, could you add a test case, just to ensure that the issue happens without your fix when running the test suite, then that your PR fixes it ? (also to prevent any future regression on this), thanks !

Also, as per our contribution guidelines, please target the dev branch for source changes

@Telroshan Telroshan added bug Something isn't working needs test PR needs a test needs rebase/retarget labels Sep 26, 2024
@jackielii jackielii changed the base branch from master to dev September 26, 2024 07:48
@jackielii jackielii changed the base branch from dev to master September 26, 2024 07:55
@jackielii jackielii changed the base branch from master to dev September 26, 2024 07:58
@jackielii
Copy link
Contributor Author

Hi @Telroshan ,

I added test, and found out about the outerHTML swap issue. So fixed that and added a test for it as well.

Let me know if you need anything else.

@Telroshan
Copy link
Collaborator

Thanks, though if I run your tests locally without your PR's fix (thus with the current library's state), the test seem to already pass 😬
image

The tests should be setup so that they fail with the current state of the lib, and pass with your fix (to prove the resolution, and ensure we don't introduce regressions in the future).
So in their current state, the 2 new tests appear to be "useless" in the way that they don't fail with the buggy lib behavior in the first place.

It's weird because I don't seem to even get an exception in the console. Didn't spend time investigating this so I can't be of much more help right now unfortunately!

@jackielii
Copy link
Contributor Author

Hmm, that's weird, I clearly run the tests without the fix and they fail. Maybe it's my last minute clean up. Will check and modify

@jackielii
Copy link
Contributor Author

hi @Telroshan , I realised I removed the div.remove() because I thought remove parent would remove the div. Turns out not to be the case. so the div.remove() is needed in the test. Should be all good now

@Telroshan Telroshan removed the needs test PR needs a test label Sep 28, 2024
Copy link
Collaborator

@Telroshan Telroshan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep all good, tests correctly fail now with the current lib's state, then pass with your fix, thanks!

@Telroshan Telroshan added the ready for review Issues that are ready to be considered for merging label Sep 28, 2024
@1cg 1cg merged commit 4916ce4 into bigskysoftware:dev Oct 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready for review Issues that are ready to be considered for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using hx-swap="delete swap:1s", element is possibly deleted
3 participants