Skip to content

Conversation

@HeavensRegent
Copy link
Contributor

Updated the createBasicRecord to also verify the parent isShown. Before it only checked to see if the parent was open.

Updated the createBasicRecord to also verify the parent isShown. Before it only checked to see if the parent was open.
@HeavensRegent HeavensRegent mentioned this pull request Mar 2, 2021
Copy link
Owner

@Lodin Lodin left a comment

Choose a reason for hiding this comment

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

Hi @HeavensRegent, thanks for the contribution! Sorry for the late response, I'm kind of busy in the latest couple of months.

Could you also add tests for the change?

@HeavensRegent
Copy link
Contributor Author

I don't quite understand how to replicate the issue on the code side... more like I have no clue. I'm having a hard time making a test that fails on the current code and is being resolved on the proposed change.

The 2 main questions I have: I'm not sure how to trigger an async reload and I'm not sure how to access the property isShowing

@HeavensRegent
Copy link
Contributor Author

@Lodin Here is an attempt at updating the tests to verify the code I added. It doesn't work, but it's as far as I could get with my current understanding. https://github.com/HeavensRegent/react-vtree/tree/tests

Copy link
Owner

@Lodin Lodin left a comment

Choose a reason for hiding this comment

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

Sorry for the long response, that's really a busy time for me and I'm kinda outside of this project. I'm going to accept this PR without tests for now because the bug needs to be fixed. If you figure out the correct tests, feel free to open a new PR for that.

@Lodin Lodin merged commit 098e3be into Lodin:master Mar 20, 2021
@Lodin
Copy link
Owner

Lodin commented Mar 20, 2021

Released in react-vtree@3.0.0-beta.1. Note that now you need to use the beta tag to install the 3.0.0 version because I have released 3.0.0-beta.0 as latest by mistake.

Sorry again for the mess with the bugfix.

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.

2 participants