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

[plugin-ext]: Modal dialog enhancements #8090

Closed
kittaakos opened this issue Jun 25, 2020 · 3 comments · Fixed by #10245
Closed

[plugin-ext]: Modal dialog enhancements #8090

kittaakos opened this issue Jun 25, 2020 · 3 comments · Fixed by #10245
Labels
dialogs issues related to dialogs help wanted issues meant to be picked up, require help plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility

Comments

@kittaakos
Copy link
Contributor

kittaakos commented Jun 25, 2020

Bug Description:

I found a few issues with the modal notification dialog when I was reviewing #8080

Steps to Reproduce:

Additional Information

  • Operating System:
  • Theia Version:
@kittaakos kittaakos changed the title [plugin-ext]: Model dialog enhancements [plugin-ext]: Modal dialog enhancements Jun 25, 2020
@tetchel
Copy link
Contributor

tetchel commented Jun 25, 2020

to the point about the modal title, obviously it would be nice if extensions could configure the title, but the VS Code API doesn't allow that (there is no title). So since you want to keep your API consistent with theirs, maybe the title could be Information | Warning | Error

@tetchel
Copy link
Contributor

tetchel commented Jun 25, 2020

And for the button colour, it's always marked as a 'secondary' button:
https://github.com/eclipse-theia/theia/blob/master/packages/core/src/browser/dialogs.ts#L198

but it should only be 'secondary' if at least one other button is present on the modal.

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Jun 25, 2020

to the point about the modal title, obviously it would be nice if extensions could configure the title, but the VS Code API doesn't allow that (there is no title). So since you want to keep your API consistent with theirs, maybe the title could be Information | Warning | Error

@tetchel for the title, I believe the idea would be to use the applicationName (similarly to other places) and not hardcode the value 'Theia'.

protected applicationName = FrontendApplicationConfigProvider.get().applicationName;

@vince-fugnitto vince-fugnitto added dialogs issues related to dialogs plug-in system issues related to the plug-in system labels Jun 25, 2020
@akosyakov akosyakov added help wanted issues meant to be picked up, require help vscode issues related to VSCode compatibility labels Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialogs issues related to dialogs help wanted issues meant to be picked up, require help plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants