Hook up chamfer UI with AST-mod #4694
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
max-mrgrsk
added
feature
| icon: 'chamfer3d', | ||
| status: 'kcl-only', | ||
| status: DEV ? 'available' : 'kcl-only', |
There was a problem hiding this comment.
(Not specific to this PR, no change requested here)
@franknoirot I think we might want to start checking against another variable than DEV here? Something that could resolve to true on app.zoo.dev and nightly builds, but not release builds. Seems to me that DEV might be a tad too restrictive here. But I could be wrong too.
There was a problem hiding this comment.
Nice! I’d be happy to follow this path for fillets and chamfers while waiting for the engine team to unblock them for revolves.
Overall, I think shipping semi-working features for nightly builds could be a good practice, as we have enough warning messages in place. Including the Command Bar UI warnings. We could add an extra indication for the buttons that are "nightly only".
Besides, DEV blocks part of our team like Nick and Josh from checking and testing new stuff.
On the other hand, it doesn’t fully align with the general rule: “Don’t ship shit.” : D
There was a problem hiding this comment.
I like this idea, we should add another environment variable for this. @pierremtb would you be able to do that?
There was a problem hiding this comment.
For sure! And should be a separate PR too
| if (err(applyEdgeTreatmentToSelectionResult)) | ||
| return applyEdgeTreatmentToSelectionResult | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises |
There was a problem hiding this comment.
We're trying to prevent using these ESLint disabling comments in any new code. Would you mind converting this AST chamfer from an XState action, which is inherently synchronous, to an actor, which can be asynchronous so that we can appropriately await things like this?
There is a fresh example here from Pierre's shell PR. As you can see, it requires adding a bit of extra code and another state for the machine to sit in while the async code runs. I'm happy to pair with you or do a video walkthrough of Pierre's code if you need!
Then we can go back and refactor the Fillet code in another small PR.
There was a problem hiding this comment.
[optional] we can come back and rewrite this test in our new fixture-based approach later, but since I'm already asking you to switch out the XState actor to an action I figure I'd give you a heads up on this one too.
Pierre has a good example of a Playwright test that uses our new fixture-based approach here in his Shell PR, and we're going to try to make any new tests use it, not least of all because Lee's work on migrating our tests to Electron uses it and then all our tests can run on both desktop and browser at-will.
I can show you how to rewrite this test in the new style if you need a hand or anything, although I think it would be pretty trivial for you!
There was a problem hiding this comment.
Good poits, thanks. Is it ok if I'll do that in the following pr so I can tackle one thing at a time ? Then I can fix both chamfer and fillet in one batch
There was a problem hiding this comment.
I've seen Pierre's states too and was curious about it. Thanks for clarifying:)
There was a problem hiding this comment.
@franknoirot I've created an issue for it (#4749). The current chamfer PR will wait until the older fillet code is updated to the latest XState requirements. Once that's done, I'll base current chamfer PR on the updated code so we don't mix everything into a single pull request.
Description:
This PR adds the chamfer button to the command bar and integrates it with the AST mod. It also updates playwright tests to ensure the chamfer and button’s enabled/disabled states work correctly under various conditions.
Todo:
Screen.Recording.2024-12-08.at.18.44.22.mov
References