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 command to add an auto-detected project as a favorite #229

Merged
merged 1 commit into from
Dec 6, 2018
Merged

Add command to add an auto-detected project as a favorite #229

merged 1 commit into from
Dec 6, 2018

Conversation

ckaczor
Copy link
Contributor

@ckaczor ckaczor commented Oct 17, 2018

I have a bunch of auto-detected projects and I wanted a quick way to add some as a favorite. This adds a context menu item that will quickly save a project as a favorite.

@alefragnani
Copy link
Owner

Hi @ckaczor ,

First of all, thanks for you PR and sorry for taking so long to answer. I was a bit busy in the last couple of weeks, with my vacation and the preparations for speaking at Embarcadero Conference.

About your PR, I think that instead of a new command, you should reuse the Save Project command, because it would still ask the user for the project name.

Of course the saveProject() function itself should be updated to accept another path (today it uses vscode.workspace.workspaceFolders[0] only), but I think the changes would be fairly easy to do.

Could you update the PR for that?

Thank you

@ckaczor
Copy link
Contributor Author

ckaczor commented Oct 30, 2018

Thanks for the feedback - I updated the commit with the requested changes.

Copy link
Owner

@alefragnani alefragnani left a comment

Choose a reason for hiding this comment

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

Both calls to showStatusBar (lines 304 and 325) should not occur if node is not undefined. Otherwise, it will incorrectly update the status bar to show the "saved project", which in this case, is not the active

src/extension.ts Outdated Show resolved Hide resolved
@alefragnani
Copy link
Owner

Hi @ckaczor ,

I'm finally returning to the PRs, and made two comments on that. Could you take a look?

My intention is to start releasing updates in the weekend.

Thank you

@alefragnani alefragnani added the PR needs update The PR needs review by the author label Nov 28, 2018
@ckaczor ckaczor closed this Nov 30, 2018
@ckaczor ckaczor reopened this Nov 30, 2018
@ckaczor
Copy link
Contributor Author

ckaczor commented Nov 30, 2018

Changes should be all set - thanks for the feedback =)

@alefragnani alefragnani removed the PR needs update The PR needs review by the author label Dec 4, 2018
@alefragnani alefragnani added this to the 10.1.0 milestone Dec 5, 2018
@alefragnani alefragnani merged commit c518cdd into alefragnani:master Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants