-
Notifications
You must be signed in to change notification settings - Fork 52
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
Allow multiple profiles in the same sketch #5196
Conversation
This reverts commit efe8089.
* Add Rust side artifacts for startSketchOn face or plane * move ast digging --------- Co-authored-by: Kurt Hutten Irev-Dev <[email protected]>
…e not ready with name params yet
…riteToFile` It is already debounced internally. If we await it, we will have to wait for a debounced timeout
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.
Went through the changes at a high level. Kudos for getting this to a mergeable state. Local testing looks good to me too ❤️
Waiting on @franknoirot's approval before merging
}, | ||
], | ||
// TODO FIXME, similar to fix me in e2e/playwright/testing-selections.spec.ts | ||
// also related to deleting, deleting in general probably is due for a refactor |
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.
Agreed!
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.
Just noticed this deletion of Sketch in the feature tree leads to the sketch var to be delete but not the profiles in it. This can totally be fixed in a subsequent PR, I can take care of opening up a new issue for this.
Screen.Recording.2025-02-14.at.8.43.35.AM.copy.mov
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.
As discussed on Slack @franknoirot
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.
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.
Way to go getting this across the finish line team.
This is ready I think, battling tests a little bit, but that shouldn't hold up reviews
Refer to this PR for the main PR description.
#4532
Things that are different from that PR is that three-point-circle had some work done with it, it also required that each segment now has many groups of overlays to account for the three handles of three-point-circle that all have x-y inputs
![image](https://private-user-images.githubusercontent.com/29681384/411533827-f0a9e898-54ae-4847-87fc-0cc272f5ea00.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk1NTIwMDgsIm5iZiI6MTczOTU1MTcwOCwicGF0aCI6Ii8yOTY4MTM4NC80MTE1MzM4MjctZjBhOWU4OTgtNTRhZS00ODQ3LTg3ZmMtMGNjMjcyZjVlYTAwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE0VDE2NDgyOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPThhOGE4MzEyMmQyODA1ZmJmODM1NTBmMzI4YjJmMTA0MTI0M2M2MzA1MmYxMmY0MDM3ZWE1ZTg2NmYyYTQyODUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.aJb0fIA5DQHfGddx3U_3lcxs_djJZ4IvyRBGPLUfpdA)
There is some kwarg related work in there two.
closes #4828
closes #4619
closes #5191
closes #4233
closes #2931
closes #3082
closes #1876
closes #2426
closes #589