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

CHE-9525 add modal notifications #1844

Closed
wants to merge 1 commit into from
Closed

CHE-9525 add modal notifications #1844

wants to merge 1 commit into from

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented May 3, 2018

Add modal notifications.
It needs to realize for API for #1482
This modal notification message optionally provide an array of items which will be presented as clickable buttons and can be shown using the warn,
info and error functions in MessageService.

Simple example that show a modal information message:

messageService.info('Information message', { modal: true });

Simple example that show a modal information message with buttons:

messageService.info('Information message', { modal: true }, 'Btn1', 'Btn2').then(result => {
    console.log("Click button", result);
});

selection_141
selection_148
selection_154
Both kind of notifications with buttons:
selection_136

Signed-off-by: Oleksii Orel oorel@redhat.com

Signed-off-by: Oleksii Orel <oorel@redhat.com>
@svenefftinge
Copy link
Contributor

Why do they need to be modal?

@gorkem
Copy link
Contributor

gorkem commented May 4, 2018

@olexii4 I do not like these old fashioned dialogs that appear in the middle of the screen at all. I think it will be better if we do not introduce those. /cc @slemeur

@svenefftinge modality has its uses, for instance che workspace containers are shutdown on long periods of inactivity and when that happens we display a modal dialog with start button to allow the user to return immediately.

@benoitf
Copy link
Contributor

benoitf commented May 4, 2018

FYI modal message windows are available to vscode message API
https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts#L1709-L1715

thus ppl that want to convert plugins from vscode to theia will miss that feature if there is no modal option.

@AlexTugarev
Copy link
Contributor

@benoitf that's the point, they support it in the window API, which is very high level and not to be confused with the notification api in both, vscode and theia. the window messages API will delegate to two different services, one for notification, and the other for dialogs [1]. I'm not aware of plans for a dialog service in theia, but I think that modal dialog feature should belong there.

[1] https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/electron-browser/mainThreadMessageService.ts#L37-L43

@AlexTugarev
Copy link
Contributor

Besides putting it into a dedicated service, I'm curious how this implementation would behave. Let's say there are multiple request to open a modal dialog, would that result in having a visible stack of dialogs? or would they appear one after the other?

@AlexTugarev
Copy link
Contributor

@benoitf, @olexii4, the MessageService API in theia is inspired by INotificationService, which is a "service to bring up notifications and non-modal prompts."

https://github.com/Microsoft/vscode/blob/master/src/vs/platform/notification/common/notification.ts#L157

@AlexTugarev
Copy link
Contributor

I looked for usages of modal option in vscode, and it seems only the git extension is using it for confirmation commands, which seems to be reasonable.

@benoitf
Copy link
Contributor

benoitf commented May 4, 2018

@AlexTugarev so you would like to have DialogService which bring dialogs/modals ?
https://github.com/Microsoft/vscode/blob/684af0064b3c34f9b6bfae1664ae2c239a468865/src/vs/platform/dialogs/common/dialogs.ts ?

and window.showMessage delegating to either MessageService or DialogService ?

@AlexTugarev
Copy link
Contributor

Do you aim to introduce the window API as part the plugin story? I'm not aware of such plans so far.

On the DialogService , I think it would make sense to have one. @svenefftinge WDYT?

@svenefftinge
Copy link
Contributor

We have dialogs already : https://github.com/theia-ide/theia/blob/master/packages/core/src/browser/dialogs.ts

It is not a service but an abstract class, which might be a bit more inconvenient but provides more control, which IMHO is a good thing in this case.

@benoitf
Copy link
Contributor

benoitf commented May 4, 2018

so, to sum up:

 window.showMessage --> 
  if not modal ==> notificationService
  if modal ==> Dialog

?

@olexii4
Copy link
Contributor Author

olexii4 commented May 4, 2018

Hello
Our team has started work on our windows API here - https://github.com/theia-ide/theia/blob/isolated-extension-api/packages/plugin/src/theia.d.ts#L248
I have read all comments. As I understand it, you don't mind the window modal notification API which is related to vscode message API in general, right?
Can you please advice what should I do then?

@AlexTugarev
Copy link
Contributor

@benoitf, that would correspond with vscode, right?

@benoitf
Copy link
Contributor

benoitf commented May 4, 2018

@AlexTugarev yes

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.

None yet

5 participants