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

Replace prompt with quickpick #14885

Merged
merged 2 commits into from
Dec 4, 2020
Merged

Replace prompt with quickpick #14885

merged 2 commits into from
Dec 4, 2020

Conversation

joyceerhl
Copy link

Addressing UX feedback from yesterday's PyTorch tooling sync. Change the logdir request flow to use a quickpick instead of a prompt, as it's likely that users will miss the prompt (which they must currently acknowledge before we proceed to start a session).

image

  • 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).
  • The wiki is updated with any design decisions/details.

@codecov-io
Copy link

codecov-io commented Dec 4, 2020

Codecov Report

Merging #14885 (6cfb978) into main (d88c119) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##            main   #14885   +/-   ##
======================================
  Coverage     65%      65%           
======================================
  Files        551      551           
  Lines      25896    25901    +5     
  Branches    3673     3673           
======================================
+ Hits       16905    16910    +5     
  Misses      8298     8298           
  Partials     693      693           
Impacted Files Coverage Δ
src/client/common/utils/localize.ts 91% <100%> (+<1%) ⬆️

const selection = await window.showInformationMessage(message, ...prompts);
switch (selection) {
case gotIt:
const selectAnotherFolder = TensorBoard.selectAnotherFolder();
Copy link

Choose a reason for hiding this comment

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

Do we want a don't ask me again? I kept getting this prompt every time I opened a folder.

Copy link
Author

@joyceerhl joyceerhl Dec 4, 2020

Choose a reason for hiding this comment

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

So there are 2 prompts. The prompt you're thinking of is the 'VSCode now has native TensorBoard support' prompt, which does have a do not ask again option that permanently disables it. The other prompt (the one in this PR) is being removed in this PR because we decided users were going to miss it.

Copy link
Author

Choose a reason for hiding this comment

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

This one:

case doNotAskAgain:
await this.disablePrompt();

Copy link

Choose a reason for hiding this comment

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

Okay maybe it wasn't clear to me that I needed to disable the first prompt in order for the second one to stop appearing. I wanted to have the first prompt show up all the time (so that if I actually wanted to see the tensor board at some later date it would ask me again), but the second one kept asking me the same question over and over. I didn't want to have to answer the second question the same way more than once.

Copy link
Author

@joyceerhl joyceerhl Dec 4, 2020

Choose a reason for hiding this comment

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

Oh I see, yeah there wasn't a way to disable the the second prompt (the one about using the cwd for the log directory). But that prompt is being removed. And yes the first prompt is being changed to show up all the time now (with a do not show again button for when the user gets sick of seeing it)!

We do still need to ask the user for the log directory on each session launch, because we need to point tensorboard to the logfiles. Maybe we can add a workspace-level setting to 'Always use this log directory'? /cc @jmew

@joyceerhl joyceerhl merged commit 3698950 into main Dec 4, 2020
@joyceerhl joyceerhl deleted the tensorBoardQuickPick branch December 4, 2020 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants