Skip to content

Conversation

@minsa110
Copy link
Contributor

For https://github.com/microsoft/vscode-jupyter-internal/issues/243

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).

@minsa110 minsa110 marked this pull request as ready for review January 19, 2022 20:50
@minsa110 minsa110 requested a review from a team as a code owner January 19, 2022 20:50
"id": "jupyterWelcome",
"title": "Get started with Jupyter Notebooks",
"description": "Your first steps to set up a Jupyter project with all the powerful tools and features that the Jupyter extension has to offer!",
"when": "false",
Copy link
Contributor

Choose a reason for hiding this comment

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

when: false? Does this mean it doesn't show up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, by default it's turned off. The experiment turns it on for folks:

vscode.gettingStarted.overrideCategory.ms-toolsai.jupyter.jupyterWelcome.when:workspacePlatform != webworker

package.json Outdated
"tslint-plugin-prettier": "^2.1.0",
"typemoq": "^2.1.0",
"typescript": "^4.4.3",
"typescript": "^4.5.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

THis seems like an odd addition? Did you need a new typescript compiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for a new TS compiler. I think it just updated it with the one I used.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should back out this change then. This should also remove the change to package-lock.json

@rchiodo
Copy link
Contributor

rchiodo commented Jan 19, 2022

Could you screenshot what it actually looks like and paste it here? It's hard to get a feel for the walkthrough just by looking at the package.json.

@rchiodo
Copy link
Contributor

rchiodo commented Jan 19, 2022

I was also wondering how the walkthrough entries get translated to other languages? Is there support for loading the strings in them from an nls.json file?

@minsa110
Copy link
Contributor Author

minsa110 commented Jan 19, 2022

Here is a screenshot (can only expand one step at a time, so only the first one is expanded in the screenshot):
image

You can also see a gif of it here: https://github.com/microsoft/vscode-jupyter-internal/issues/243

"title": "Create or open a Jupyter Notebook",
"description": "Right click in the file explorer and create a new file with an .ipynb extension. Or, open the [Command Palette](command:workbench.action.showCommands) and run the command \n``Jupyter: Create New Blank Notebook``.\n[Create new Jupyter Notebook](command:toSide:jupyter.createnewnotebook)\n If you have an existing project, you can also [open a folder](command:workbench.action.files.openFolder) and/or clone a project from GitHub: [clone a Git repository](command:git.clone).",
"media": {
"image": "images/OpenOrCreateNotebook.gif",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think i remember VS Code adding support for themable images (svgs) for welcome pages.
Else someone on a light them (default) would get dark images.
@luabud might have more information about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, do we need gifs for everything?
Python extension has SVGs and I personally prefer that, rather than just having dark images

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit torn myself. The SVGs look much better, but at the same time the gifs feel like they explain things better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, but for some like opening a notebook, I don't think we need a gif,
also thats the very first walkthrough, i'd like that to be themeble via svg

Copy link
Contributor Author

@minsa110 minsa110 Jan 20, 2022

Choose a reason for hiding this comment

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

Yea, I think Python walkthrough pages use them. I was thinking that once we start seeing positive numbers from the experiment, we'd ask Miguel to help us make them for us as well (I don't have the artsy skills to create svgs 😅). This way we can get the bits in front of our users fast and start getting statistically significant improvements to our feature usage and/or engagement to justify our request. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that personally, you can track it how you want, but might be good to create an issue for that change of adding SVGs, just so it doesn't get forgotten in day to day shuffle..

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with Ian, approving

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks folks! #8782

Copy link
Contributor

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Suggest we use SVGs where possible like Python extension to cater all users with different themes

@minsa110
Copy link
Contributor Author

Hey @rchiodo, sorry I missed your other reply. It is currently not being localized and we plan on running the experiment for users who have English set as their default language. I don't think this has been addressed by other extensions either with exception to this one that C++ team uses

}
},
"typescript": {
"version": "4.4.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

This change needs to be reverted too. Otherwise it doesn't match the package.json.

@DonJayamanne DonJayamanne self-requested a review January 20, 2022 18:34
@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2022

Codecov Report

Merging #8733 (eb90a8a) into main (70cb636) will increase coverage by 0%.
The diff coverage is n/a.

❗ Current head eb90a8a differs from pull request most recent head 9227312. Consider uploading reports for the commit 9227312 to get more accurate results

@@          Coverage Diff           @@
##            main   #8733    +/-   ##
======================================
  Coverage     71%     72%            
======================================
  Files        381     385     +4     
  Lines      24484   24819   +335     
  Branches    3755    3822    +67     
======================================
+ Hits       17508   17888   +380     
+ Misses      5465    5370    -95     
- Partials    1511    1561    +50     
Impacted Files Coverage Δ
...ience/variablesView/variableViewMessageListener.ts 77% <0%> (-23%) ⬇️
...ent/common/application/webviewViews/webviewView.ts 67% <0%> (-11%) ⬇️
...rc/client/datascience/errors/errorRendererComms.ts 25% <0%> (-9%) ⬇️
...ent/datascience/jupyter/pythonVariableRequester.ts 71% <0%> (-6%) ⬇️
...ience/kernel-launcher/localKernelSpecFinderBase.ts 74% <0%> (-3%) ⬇️
src/client/datascience/jupyter/kernelVariables.ts 59% <0%> (-2%) ⬇️
src/client/extensionActivation.ts 94% <0%> (-2%) ⬇️
...ient/datascience/kernel-launcher/kernelLauncher.ts 81% <0%> (-2%) ⬇️
src/client/datascience/jupyter/kernels/kernel.ts 75% <0%> (-2%) ⬇️
...tascience/jupyter/kernels/kernelCommandListener.ts 70% <0%> (-1%) ⬇️
... and 40 more

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.

7 participants