-
Notifications
You must be signed in to change notification settings - Fork 79
#4852 Delete line segments in sketch mode using keyboard #6407
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
#4852 Delete line segments in sketch mode using keyboard #6407
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Excited for this! |
Update: since this is not a visual change, I'll continue after #6333 but the direction we seem to prefer is to implement this in the React component which implements the existing delete segment function in the "..." options menu. |
Small usability question: in this sketch of a triangle with 3 lines, when I delete the last line, it will actually not change anything because of the
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6407 +/- ##
=======================================
Coverage 85.31% 85.31%
=======================================
Files 110 110
Lines 47356 47356
=======================================
Hits 40403 40403
Misses 6953 6953
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
The changes make sense, and testing it locally this feels really good too! I didn't mind deleting twice, it's definitely a huge improvement already and it made sense to me that one delete led to the deletion of one line. I think people are used to hitting delete multiple times too.
@pierremtb thanks! I've added a quick test for this to in a separate PR: |
@Irev-Dev this is just a quick try to see how to approach this, it is almost working, but I wonder.. isn’t the main issue here that
modelingSend({ type: 'Delete selection' })
is not handling the selected segments? Or is that only supposed to delete larger scoped items in the feature tree, like sketches?This PR just checks if the current selection is a segment and calls the segment specific
Delete Segment
command in that case..the reason it's not working yet is that the artifact is not found for segments just drawn, it is found when we exit and re-enter the sketch - probably the artifact graph needs to be refreshed.My other approach would be to add a hotkey listener in the React component where the current delete segment button UI is..but having one delete callback which deletes the current selection seems cleaner here.