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

Initial share provider API and UI #182999

Merged
merged 6 commits into from May 22, 2023
Merged

Initial share provider API and UI #182999

merged 6 commits into from May 22, 2023

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented May 19, 2023

For #176316

Introduces a share provider API and a top-level Share... command and icon in the command center toolbar. There's currently only one provider (from the builtin GitHub extension), more to come. Everything is hidden behind an unofficial setting workbench.experimental.share.enabled, demo:

share.mp4

Screenshot:
image

@joyceerhl joyceerhl marked this pull request as ready for review May 19, 2023 21:59
@joyceerhl joyceerhl requested a review from bpasero May 19, 2023 21:59
@VSCodeTriageBot VSCodeTriageBot added this to the May 2023 milestone May 19, 2023
@@ -154,7 +154,12 @@ export interface IPromptResultWithCancel<T> extends IPromptResult<T> {

export type IDialogResult = IConfirmationResult | IInputResult | IPromptResult<unknown>;

export type DialogType = 'none' | 'info' | 'error' | 'question' | 'warning';
export type DialogType = 'none' | 'info' | 'error' | 'question' | 'warning' | 'success';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bpasero this PR introduces a success/confirmation dialog to support a Share experience, given prior art in Powerpoint/Word, e.g.:
image

This also addresses a pain point where copy link actions today do not display success/confirmation and can fail silently. I would appreciate your review of the dialog-related changes here please--thank you!

@joyceerhl joyceerhl changed the title Initial share provider API Initial share provider API and UI May 19, 2023
@abhijit-chikane
Copy link
Contributor

I think clicking on share from command palette should directly open that dialog

@joyceerhl
Copy link
Contributor Author

@abhijit-chikane yes, it does, I just didn't include that in the recording (the recording shows that Share... exists in the palette and that there is a button to access it which does the same thing)

@bpasero bpasero requested review from sbatten and removed request for bpasero May 20, 2023 04:22
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 find the introduction of a Success severity strange tbh. I think the current values for Severity make sense, but Success is not a severity for me.

@joyceerhl
Copy link
Contributor Author

@bpasero would it help to rename Success to Confirmation, or to rename Severity to Reason?

@bpasero
Copy link
Member

bpasero commented May 22, 2023

I think severity is used in many contexts, but for dialogs specifically maybe something else should be used. Here is what Electron calls it:

/**
* Can be `"none"`, `"info"`, `"error"`, `"question"` or `"warning"`. On Windows,
* `"question"` displays the same icon as `"info"`, unless you set an icon using
* the `"icon"` option. On macOS, both `"warning"` and `"error"` display the same
* warning icon.
*/
type?: string;

@joyceerhl
Copy link
Contributor Author

@bpasero would it make sense for Success to be a subtype of the info type, which is internally treated differently for dialogs just to distinguish what codicon to use?

@bpasero
Copy link
Member

bpasero commented May 22, 2023

@joyceerhl can you talk with SteVen as component owner of custom dialogs? Maybe someone else took that over, not sure, he would know.

@joyceerhl joyceerhl enabled auto-merge (squash) May 22, 2023 18:53
@joyceerhl joyceerhl removed the request for review from sbatten May 22, 2023 21:42
@joyceerhl joyceerhl merged commit bc1090c into main May 22, 2023
6 checks passed
@joyceerhl joyceerhl deleted the dev/joyceerhl/inc-vole branch May 22, 2023 22:32
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 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.

None yet

5 participants