-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Deleting a branch calls dialog of deleting its worktree (GLVSC-632) #3509
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
Deleting a branch calls dialog of deleting its worktree (GLVSC-632) #3509
Conversation
ce5c2d9
to
4d7a322
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.
Just commenting on the user experience, but when I choose to delete a branch, I find it a bit stange that it jumps me into this screen:
There is no indication here that "this is a branch with a worktree, so you need to delete the worktree first". It just says "delete worktree". So even though I know what is happening as a developer, if I was a user who is unaware of what is going on, there is no information on what's going on here, so it would feel like I clicked the wrong command, even though I did not.
I think we can do a better job with messaging to make this less confusing. Either the title of the "delete worktree" step can be updated, or the two available options can say "Delete Worktree & Branch" and "Force Delete Worktree & Branch", to make it clear that we are deleting the worktree and then the branch.
src/commands/git/worktree.ts
Outdated
repo: string | Repository; | ||
uris: Uri[]; | ||
flags: DeleteFlags[]; | ||
doNotSuggestDeletingBranches?: boolean; |
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.
Maybe we can just pass branch
or branches
as state when coming from "delete branch" and then we don't need to calculate them on the delete steps, and also can use their existence in state as a flag to restrict the options?
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.
then we don't need to calculate them on the delete steps
It's not clear how we can use the passed branches on the "worktrees" step. We calculate branches only if "Delete Worktrees&Branches" is selected, but those are hidden.
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.
Fair enough - if the state isn't needed then a boolean should suffice 👍🏼
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.
I'm thinking about renaming because it affects the view in a very specific case. "Deleting of the worktree of a selected branch". So, it can be called like this: deletingOfSelectedBranches
, or moreover I'm thinking of replacing the field by a flag:
type DeleteFlags = '--force' | '--delete-branches' | '--deleting-of-selected-branches';
What do you think?
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.
@sergeibbb No preference here. Up to you. Doing it with a flag like that sounds fine. My main hope is that the messaging on the worktree step allows me to understand that we are deleting the worktree because it is necessary to delete the branch.
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.
Ok. I'm leaving it with the flag. Feel free to suggest updates for the wording, so it would be more clearer for user. You are the native speaker, so, you know it better.
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.
@sergeibbb I think we can take out the word "corresponding" in the options since it becomes a bit wordy. We should also make sure the capitalization is consistent in the options.
What do you think about "Delete Worktree of Branch"/"Delete Worktrees of Branches" etc.?
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.
@sergeibbb I updated the wording on your branch. Thanks for working on this!
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.
Great! Thank you!
7a5d919
to
c029552
Compare
4d7a322
to
34d4ec0
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.
LGTM
Description
Or instead, the Delete Worktree flow could just be triggered as an optional step in the Delete Branch flow.
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses