Conversation
| etag = this.lastResolvedDiskStat.etag; // otherwise respect etag to support caching | ||
| } | ||
|
|
||
| const storageKey = 'workbench.tasks.ranTaskBefore'; |
There was a problem hiding this comment.
Can you put this in its own method?
| private taskNotification(): void { | ||
| const storageKey = 'workbench.tasks.ranTaskBefore'; | ||
| const fileName = path.relative(this.storageService['workspaceKey'], this.resource['_formatted']); | ||
| const fileName = path.relative(this.contextService.getWorkspace().resource.toString(), this.resource.toString()); |
There was a problem hiding this comment.
Can you only do this check if the storageService check succeeds, to avoid extra work, since most of the time, it won't be needed?
| etag = this.lastResolvedDiskStat.etag; // otherwise respect etag to support caching | ||
| } | ||
|
|
||
| this.taskNotification(); |
There was a problem hiding this comment.
Maybe this should only happen after opening the file succeeds? Partly to avoid slowing down the file open with extra work, but also, a file can fail to open, in which case you don't want to show the notification.
There was a problem hiding this comment.
Where are the extension recommendations being shown from?
| .then(content => this.handleLoadSuccess(content), error => this.handleLoadError(error)); | ||
| } | ||
|
|
||
| private taskNotification(): void { |
There was a problem hiding this comment.
More verb-y name like showTaskNotification or something
|
@chrmarti is this how you did AB testing for the welcome page? |
There was a problem hiding this comment.
You can use telemetryService.getExperiments().showTaskDocumentation instead.
There was a problem hiding this comment.
This is not needed then (see below). The welcome page one (you probably looked at) was a special case I'm about to remove.
There was a problem hiding this comment.
Gotcha. Thanks Christof :D . You're the best!
There was a problem hiding this comment.
This is going to fail the main build, or not work - localized strings have to be constant string literals. But you can add a placeholder like this:
localize('id', 'foo {0} bar {1}', 'string inserted at {0}', 'inserted at {1}' ...)
There was a problem hiding this comment.
Ah that's actually what travis is complaining about: https://travis-ci.org/Microsoft/vscode/jobs/244707435#L3648
There was a problem hiding this comment.
I think the product name should be "VS Code"
There was a problem hiding this comment.
Type as Action instead of any
There was a problem hiding this comment.
mfw you have to import it
There was a problem hiding this comment.
Code style wise, this is totally fine, even though it feels weird to declare action and messageTest above then duplicate the assignments like this. But JS doesn't really give you better tools and this is one way I would write it.
Lixire
force-pushed
the
amqi/experiment-1-4
branch
from
1506017 to
f7d900d
Compare
- when user opens grunt/gulp/tsconfig file and hasn't run a task, points user to command palette (>tasks) - add common actions for quickopen
Lixire
force-pushed
the
amqi/experiment-1-4
branch
from
28a1b9d to
f6a02f8
Compare
4 participants
Adds a notification when a grunt/gulp/tsconfig file is opened to check out tasks. Uses the memento from before to not bother people who have already run tasks.