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

Welcome Dialog Service #176580

Merged
merged 10 commits into from
Mar 10, 2023
Merged

Welcome Dialog Service #176580

merged 10 commits into from
Mar 10, 2023

Conversation

bhavyaus
Copy link
Collaborator

@bhavyaus bhavyaus commented Mar 9, 2023

Related: #166895
The welcomeDialogService uses the existing

export class Dialog extends Disposable {

to create a welcome modal dialog.

image

@vscodenpa vscodenpa added this to the March 2023 milestone Mar 9, 2023
@bhavyaus bhavyaus requested a review from joyceerhl March 9, 2023 01:12
joyceerhl
joyceerhl previously approved these changes Mar 9, 2023
@bhavyaus bhavyaus force-pushed the dev/bhavyau/modal-dialog branch from d1c9fe3 to ba8a027 Compare March 9, 2023 01:24
@bhavyaus bhavyaus enabled auto-merge (squash) March 9, 2023 01:33
@bhavyaus bhavyaus requested a review from joyceerhl March 9, 2023 01:45
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I am a little bit confused why we now have a modalDialogService. To clarify, we already have IDialogService and it opens modal dialogs (either native or custom). Why do we need a second service?

If we can move the functionality into the IDialogService that would be better, otherwise for a developer it will be hard to tell apart dialog service from modal dialog service.

@bhavyaus
Copy link
Collaborator Author

bhavyaus commented Mar 9, 2023

I am a little bit confused why we now have a modalDialogService. To clarify, we already have IDialogService and it opens modal dialogs (either native or custom). Why do we need a second service?

If we can move the functionality into the IDialogService that would be better, otherwise for a developer it will be hard to tell apart dialog service from modal dialog service.

Fixed.
This dialog is specific to the welcome view (see related: #166895) .
Moved and renamed service to include the welcome prefix.

@bhavyaus bhavyaus requested a review from bpasero March 9, 2023 06:24
@bhavyaus bhavyaus force-pushed the dev/bhavyau/modal-dialog branch from 0386762 to c0afba8 Compare March 9, 2023 06:33
@bpasero bpasero self-requested a review March 9, 2023 15:19
bpasero
bpasero previously requested changes Mar 9, 2023
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Why not just welcomeDialog? To me, every dialog is modal by definition.

src/vs/workbench/browser/web.api.ts Outdated Show resolved Hide resolved
@bhavyaus bhavyaus disabled auto-merge March 9, 2023 15:52
@bhavyaus bhavyaus force-pushed the dev/bhavyau/modal-dialog branch from c0afba8 to 336a349 Compare March 9, 2023 16:36
@bhavyaus bhavyaus changed the title Welcome Modal Dialog Service Welcome Dialog Service Mar 9, 2023
@bhavyaus bhavyaus requested a review from bpasero March 9, 2023 16:37
@bhavyaus bhavyaus force-pushed the dev/bhavyau/modal-dialog branch from 336a349 to cc7b02e Compare March 9, 2023 16:37
src/vs/workbench/browser/web.api.ts Outdated Show resolved Hide resolved
@bhavyaus bhavyaus requested a review from joyceerhl March 9, 2023 17:16
joyceerhl
joyceerhl previously approved these changes Mar 9, 2023
joyceerhl
joyceerhl previously approved these changes Mar 9, 2023
@bhavyaus bhavyaus force-pushed the dev/bhavyau/modal-dialog branch from e5536d4 to 1e035cf Compare March 10, 2023 18:43
@bhavyaus bhavyaus requested a review from joyceerhl March 10, 2023 19:55
@bhavyaus bhavyaus dismissed bpasero’s stale review March 10, 2023 19:59

Thanks Ben. Since there are no other architecture changes, ill remove you from this PR. I did update the comment as you requested.

@bhavyaus bhavyaus merged commit 8bf0518 into main Mar 10, 2023
@bhavyaus bhavyaus deleted the dev/bhavyau/modal-dialog branch March 10, 2023 20:00
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants