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

feat: add install Java17 option when choose JavaDownload #951

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

yoshinorin
Copy link
Contributor

As is the title.

Problem

If we do not apply modal: true to showInformationMessage, users can not decide which button should select that they want to download. Because, showInformationMessage width is narrow and can not contain 4 buttons.

narrow

Solution (idea)

We can apply modal: true to showInformationMessage. This option showInformationMessage with a dialog. It can contain 4 buttons.

metals-vscode2

How do you think?

Thank you :)

@kpodsiad
Copy link
Member

I'm ok with the modal, because having a JDK is crucial for Metals and any problem related to its absence is serious, thus modal makes sense - it forces you to make some decisions.

wdyt @tgodzik

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks good, but I would even remove the option for JDK 8 to prepare for a possible deprecation.

@dos65
Copy link
Member

dos65 commented Apr 25, 2022

@tgodzik @kpodsiad
There is also an option to put these choices into quick pick menu. wdyt?

@tgodzik
Copy link
Contributor

tgodzik commented Apr 25, 2022

@tgodzik @kpodsiad There is also an option to put these choices into quick pick menu. wdyt?

I think the quick pick is not the right fir for it, it's to easy to make it disappear

@kpodsiad
Copy link
Member

@tgodzik @dos65 what is the consensus? If I understand correctly it is:

  • removing JDK8
  • using modal is fine since having JDK is crucial

@tgodzik
Copy link
Contributor

tgodzik commented Apr 26, 2022

@tgodzik @dos65 what is the consensus? If I understand correctly it is:

  • removing JDK8
  • using modal is fine since having JDK is crucial

That's my opinion here, yes.

@kpodsiad
Copy link
Member

I want to include this in the current release thus I'll commit changes that we concluded.

@kpodsiad kpodsiad requested a review from tgodzik April 26, 2022 09:14
@kpodsiad kpodsiad requested a review from dos65 April 26, 2022 11:18
Copy link
Member

@dos65 dos65 left a comment

Choose a reason for hiding this comment

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

👍

@kpodsiad kpodsiad merged commit 8795963 into scalameta:main Apr 26, 2022
@kpodsiad
Copy link
Member

Thanks @yoshinorin for your contribution!

@yoshinorin yoshinorin deleted the feat/add-java17-install branch April 26, 2022 11:41
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.

4 participants