Skip to content
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

Add confirmation for dragging and dropping project files in the "Explore" panel #31797

Closed
wants to merge 6 commits into from
Closed

Add confirmation for dragging and dropping project files in the "Explore" panel #31797

wants to merge 6 commits into from

Conversation

snhardin
Copy link

@snhardin snhardin commented Aug 1, 2017

Fixes #1414

Could use some advice here. I'm not a particularly great TypeScript developer, much less a JavaScript / electron developer. I'm trying to improve, though. Please provide detailed feedback of what needs to be changed and how my code may be improved!

@mention-bot
Copy link

@snhardin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero and @egamma to be potential reviewers.

@bpasero
Copy link
Member

bpasero commented Aug 11, 2017

@snhardin there are many users that would not want to see a confirmation dialog every time and who are perfectly fine with the way it works now. I think a better approach would be to show a dialog with a checkbox (Electron has support for this) so that a user can indicate to never be asked again. When this checkbox is enabled, we should write a setting (via IConfigurationEditingService) and never ask again. If the user feels like being prompted again, this setting can be changed again.

Does that make sense?

@bpasero bpasero added this to the On Deck milestone Aug 11, 2017
@snhardin
Copy link
Author

Yup! Makes sense! I should have something ready to look at pretty soon.

@inbrighttech
Copy link

inbrighttech commented Aug 12, 2017

@snhardin Thanks for working on this.


return onError(error, true);
});
return 0; // Not sure what to do here.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpasero I'm not finished making all my changes yet, but in the mean time, I could use some advice on what to do here. Is a return required here? If so, is there some constant I should be returning or some standard I should be following?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snhardin no, a return value should not be needed because the method is declared as

public drop(tree: ITree, data: IDragAndDropData, target: FileStat, originalEvent: DragMouseEvent): void

@bpasero bpasero modified the milestones: On Deck, Backlog Sep 1, 2017
@bpasero
Copy link
Member

bpasero commented Sep 6, 2017

@snhardin are you still working on this?

@snhardin
Copy link
Author

snhardin commented Sep 6, 2017 via email

@bpasero
Copy link
Member

bpasero commented Sep 6, 2017

Cool 👍

@microsoft microsoft deleted a comment from msftclas Sep 12, 2017
@msftclas
Copy link

msftclas commented Sep 18, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ snhardin sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -61,6 +61,8 @@ export class MessageService extends WorkbenchMessageService implements IChoiceSe
opts.checkboxLabel = confirmation.checkboxLabel;
}

confirmation.checked = false;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpasero Based on what I've read in the Electron docs, you can only check if the checkbox is checked in a message box in a callback. That's problematic since the result is delivered in a return value. I cannot return two values. Right now, I plan on just making a small callback that assigns the checkbox value to the IConfirmation object (as checked) and checking that in the drag-drop confirmation code. Is there a better way of doing this or am I on the right track?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snhardin yeah looks like. seems the only way of doing it with a checkbox result.

@bpasero
Copy link
Member

bpasero commented Oct 6, 2017

I went ahead and implemented this via a42f794

@bpasero bpasero closed this Oct 6, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There is no confirmation window while drag-dropping project files in the "Explore" panel.
6 participants