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

Created new Favorites Menu as described in the issue #884 #900

Merged
merged 6 commits into from
Nov 9, 2022

Conversation

gustavosimon
Copy link
Contributor

No description provided.

Copy link
Member

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

Thanks for contribution! Basically it works well. Some nits:

  • display command in tree item as commented in line.

Further improvements:
a) when maven.terminal.favorites is not configured, it's strange to display empty children, and you get no idea whether it's broken or how to add favorite items.
I would suggest we add a "+" button as inline action of FavoriteMenu, which provides a UI (e.g. quickpick/input) to add corresponding items.

b) Currently when I change the corresponding setting, I have to manually click the refresh button to get the tree items updated. Listening to the change and refresh tree view would be better. In fact it can be implemented by vscode.workspace.onDidChangeConfiguration API.

c) Currently the setting is shared among all projects, meaning you set favorite commands for all projects and have no change to customize it for each project. But for each project in the tree view, there is a corresponding menu, which is a little bit inconsistent and strange. Not sure whether we should allow to configure it per project.

For above further thoughts, it's ok not to address in this PR, but to create new issues to track.

src/explorer/model/FavoriteCommand.ts Show resolved Hide resolved
@gustavosimon
Copy link
Contributor Author

Hi @Eskibear ! Thanks for feedback 😀!

I will add the command in tree item as commented in line. The idea is awesome.

About a and b items, I really appreciate the idea. I think that we can create a new enhancement issue to improve this behaviors.

About c item, I just take the same behavior about the LifecycleMenu, because this commands was list for each project.
How can we track this one?

image

Btw, just for curiousity: What's the meaning of nits? It is like a note/observation?

@Eskibear
Copy link
Member

Eskibear commented Nov 8, 2022

About a and b items, I really appreciate the idea. I think that we can create a new enhancement issue to improve this behaviors.

Great!

About c item, I just take the same behavior about the LifecycleMenu

IMO there's a little difference. Items listed under LifecycleMenu are just basic phases common for every Maven project, even the simplest ones. Favorite commands are customized by users, for example I can add a "spring-boot:run" for a SpringBoot project, but it doesn't make sense to show under a non-SpringBoot project which has no spring-boot-maven-plugin configured.

Anyway, it's good and we can leave it as is now. Let's consider the improvement when we receive requests from community...

What's the meaning of nits

from "nitpicking", meaning minor things nice to improve. never mind.

@gustavosimon
Copy link
Contributor Author

Nice! I will create the issue tomorrow for we improve this implementation as described on item a and b.

IMO there's a little difference. Items listed under LifecycleMenu are just basic phases common for every Maven project, even the simplest ones. Favorite commands are customized by users, for example I can add a "spring-boot:run" for a SpringBoot project, but it doesn't make sense to show under a non-SpringBoot project which has no spring-boot-maven-plugin configured.

You're right. It makes sense. Let's consider thinking to improve this behavior to in future.

I saw that the pipelines failed. Should I do something to fix this?

Anyway, it's good and we can leave it as is now. Let's consider the improvement when we receive requests from community...

Good. Feel free to ask me for help in any issue. I daily use this extension and really would like to help to improve it more.

@Eskibear
Copy link
Member

Eskibear commented Nov 8, 2022

linting issue, you can run npm run tslint locally to do the check.

ERROR: (no-shadowed-variable) /home/runner/work/vscode-maven/vscode-maven/src/Settings.ts[66, 19]: Shadowed name: 'favorites'
ERROR: (no-trailing-whitespace) /home/runner/work/vscode-maven/vscode-maven/src/explorer/model/FavoriteCommand.ts[10, 1]: trailing whitespace
ERROR: (no-trailing-whitespace) /home/runner/work/vscode-maven/vscode-maven/src/explorer/model/FavoriteCommand.ts[23, 1]: trailing whitespace
ERROR: (no-trailing-whitespace) /home/runner/work/vscode-maven/vscode-maven/src/explorer/model/ITreeItem.ts[14, 1]: trailing whitespace
ERROR: (no-trailing-whitespace) /home/runner/work/vscode-maven/vscode-maven/src/utils/Utils.ts[262, 1]: trailing whitespace

@Eskibear
Copy link
Member

Eskibear commented Nov 8, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gustavosimon
Copy link
Contributor Author

Hello @Eskibear. Fixed linting issues. Can you trigger the pipeline again?

@Eskibear
Copy link
Member

Eskibear commented Nov 8, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Eskibear
Copy link
Member

Eskibear commented Nov 8, 2022

TS compiler also complains. If you don't mind, I can fix the errors. Then you can take your time to focus on the rest work.

@gustavosimon
Copy link
Contributor Author

Sure! Thank you @Eskibear.

@Eskibear Eskibear merged commit db028b3 into microsoft:main Nov 9, 2022
@Eskibear Eskibear added this to the November 2022 milestone Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants