-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing #13554
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
Changes from 11 commits
69951c6
53f9169
bb10e88
8366ec3
17d1b82
baa9186
f8a7937
9e034f2
ca09ca8
1ec708b
62e7bd4
f3b2e12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Update "Tip" notification for new users to either show the existing tip, a link to a feedback survey or nothing. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,31 +6,71 @@ | |
import { inject, injectable } from 'inversify'; | ||
import { IExtensionSingleActivationService } from '../../activation/types'; | ||
import { IApplicationShell } from '../../common/application/types'; | ||
import { IPersistentState, IPersistentStateFactory } from '../../common/types'; | ||
import { SurveyAndInterpreterTipNotification } from '../../common/experiments/groups'; | ||
import { IBrowserService, IExperimentService, IPersistentState, IPersistentStateFactory } from '../../common/types'; | ||
import { swallowExceptions } from '../../common/utils/decorators'; | ||
import { Common, Interpreters } from '../../common/utils/localize'; | ||
import { Common } from '../../common/utils/localize'; | ||
import { sendTelemetryEvent } from '../../telemetry'; | ||
import { EventName } from '../../telemetry/constants'; | ||
|
||
export enum NotificationType { | ||
Tip, | ||
Survey, | ||
NoPrompt | ||
} | ||
|
||
@injectable() | ||
export class InterpreterSelectionTip implements IExtensionSingleActivationService { | ||
private readonly storage: IPersistentState<boolean>; | ||
private notificationType: NotificationType; | ||
private notificationContent: string | undefined; | ||
|
||
constructor( | ||
@inject(IApplicationShell) private readonly shell: IApplicationShell, | ||
@inject(IPersistentStateFactory) private readonly factory: IPersistentStateFactory | ||
@inject(IPersistentStateFactory) private readonly factory: IPersistentStateFactory, | ||
@inject(IExperimentService) private readonly experiments: IExperimentService, | ||
@inject(IBrowserService) private browserService: IBrowserService | ||
) { | ||
this.storage = this.factory.createGlobalPersistentState('InterpreterSelectionTip', false); | ||
this.notificationType = NotificationType.NoPrompt; | ||
} | ||
|
||
public async activate(): Promise<void> { | ||
if (this.storage.value) { | ||
return; | ||
} | ||
|
||
if (await this.experiments.inExperiment(SurveyAndInterpreterTipNotification.surveyExperiment)) { | ||
this.notificationType = NotificationType.Survey; | ||
this.notificationContent = await this.experiments.getExperimentValue( | ||
SurveyAndInterpreterTipNotification.surveyExperiment | ||
); | ||
} else if (await this.experiments.inExperiment(SurveyAndInterpreterTipNotification.tipExperiment)) { | ||
this.notificationType = NotificationType.Tip; | ||
this.notificationContent = await this.experiments.getExperimentValue( | ||
SurveyAndInterpreterTipNotification.tipExperiment | ||
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the text that we show to users? Does this also consider the language setting for the text that will be displayed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I talked to @luabud about it, we are aware that this is not going to be localized. It is not something that we are concerned about for this experiment, but we are definitely going to have to keep that in mind if we plan on using this feature (setting strings in experiment data and using them) again. |
||
); | ||
} | ||
|
||
this.showTip().ignoreErrors(); | ||
} | ||
@swallowExceptions('Failed to display tip') | ||
private async showTip() { | ||
const selection = await this.shell.showInformationMessage(Interpreters.selectInterpreterTip(), Common.gotIt()); | ||
if (selection !== Common.gotIt()) { | ||
return; | ||
if (this.notificationType === NotificationType.Tip) { | ||
await this.shell.showInformationMessage(this.notificationContent!, Common.gotIt()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean to remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the new desired behaviour is to not show the prompt again when users close the prompt or ignore it, while the previous "Got it!" block would exit early without updating the stored valued that would skip showing the prompt. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to sent telemetry for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch^2, I guess I was still partly away 🧠 |
||
} else if (this.notificationType === NotificationType.Survey) { | ||
const selection = await this.shell.showInformationMessage( | ||
this.notificationContent!, | ||
Common.bannerLabelYes(), | ||
Common.bannerLabelNo() | ||
); | ||
|
||
if (selection === Common.bannerLabelYes()) { | ||
sendTelemetryEvent(EventName.ACTIVATION_SURVEY_PROMPT, undefined); | ||
this.browserService.launch('https://aka.ms/mailingListSurvey'); | ||
} | ||
} | ||
|
||
await this.storage.updateValue(true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this exported for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I thought I was going to use it in the tests 🗑️