-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Visual|Logical]Tree extensions revamp #3685
[Visual|Logical]Tree extensions revamp #3685
Conversation
Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
Switching to ready for review as the PR is up to date and all tests are passing on my end after the changes to the visual/logical tree helpers. We can always add more specific tests here later, either directly to |
6c8f1ff
to
7a8a287
Compare
7a8a287
to
f5c2f22
Compare
Started adding Unit Tests for original versions here to help us test the refactor, but hitting a deadlock after the test completes successfully. ☹ @azchohfi any tips on async ninjitsu? There's three levels of |
f5c2f22
to
865962a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the API changes @Sergio0694. It has me looking for excuses to use it XD
@RosarioPulella That's great to hear, thanks! Really happy you like these changes! 🥳 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't lose my comments when my machine turned off. Couple of small comments, but looking really good. Almost done with the original unit tests, but we should be able to start porting them over too and adding new ones.
0d7a4a4
to
3dc484c
Compare
Closes #3487
PR Type
What kind of change does this PR introduce?
Overview
There are some inconsistencies in the visual tree extensions mentioned in the linked issue, and some missing features.
This PR applies the changes mentioned in #3487 (comment).
APIs breakdown
VisualTree (click to expand):
LogicalTree (click to expand):
PR Checklist
Please check if your PR fulfills the following requirements:
Other information
Opening as draft, missing updated/revamped unit tests.