Add a 'More Information' option in telemetry popup#742
Add a 'More Information' option in telemetry popup#742aeisenberg merged 1 commit intogithub:mainfrom
Conversation
aeisenberg
force-pushed
the
aesienberg/show-info
branch
2 times, most recently
from
28bb680 to
4a50ad1
Compare
| expect(val).to.eq(true); | ||
| done(); | ||
| }); | ||
| res.catch(e => fail(e)); |
There was a problem hiding this comment.
Why isn't this catch chained onto the then above?
| }); | ||
|
|
||
| it('should show a binary choice dialog and return `yes`', (done) => { | ||
| // pretend user clicks on the url twice and then 'yes' |
There was a problem hiding this comment.
This comment doesn't match the onCall(0) - here you're just testing a direct yes click right?
| }); | ||
|
|
||
| it('should show a binary choice dialog and return `no`', (done) => { | ||
| // pretend user clicks on the url twice and then 'yes' |
There was a problem hiding this comment.
Similar: just a direct no click
| // pretend user clicks on the url twice and then 'yes' | ||
| showInformationMessageSpy.onCall(0).resolvesArg(2); | ||
| showInformationMessageSpy.onCall(1).resolvesArg(2); | ||
| showInformationMessageSpy.onCall(0).resolvesArg(3); |
There was a problem hiding this comment.
Should the last one be onCall(2)? Otherwise you're just resetting the spy.
| // pretend user clicks on the url twice and then 'yes' | ||
| showInformationMessageSpy.onCall(0).resolvesArg(2); | ||
| showInformationMessageSpy.onCall(1).resolvesArg(2); | ||
| showInformationMessageSpy.onCall(0).resolvesArg(4); |
| }); | ||
|
|
||
| it('should show a binary choice dialog with a url and return `no`', (done) => { | ||
| // pretend user clicks on the url twice and then 'yes' |
There was a problem hiding this comment.
| // pretend user clicks on the url twice and then 'yes' | |
| // pretend user clicks on the url twice and then 'no' |
| }); | ||
|
|
||
| it('should show an info dialog and confirm the action', (done) => { | ||
| // pretend user clicks on the url twice and then 'yes' |
There was a problem hiding this comment.
Here and below, please update the comments
| res.catch(e => fail(e)); | ||
| }); | ||
|
|
||
| it('should show an info dialog and not confirm the', (done) => { |
There was a problem hiding this comment.
| it('should show an info dialog and not confirm the', (done) => { | |
| it('should show an info dialog and not confirm the action if dismissed', (done) => { |
| res.catch(e => fail(e)); | ||
| }); | ||
|
|
||
| it('should show a binary choice dialog with a url and return `no`', (done) => { |
There was a problem hiding this comment.
Can you also test for the sequence url, url, dismissed?
extensions/ql-vscode/src/helpers.ts
Outdated
| if (chosenItem === urlItem) { | ||
| await env.openExternal(Uri.parse(url, true)); | ||
| } | ||
| } while (chosenItem === urlItem); |
There was a problem hiding this comment.
Reading this I am worried about it creating dialogs in a loop.
This works for modal dialogs because VS Code doesn't create a fresh dialog each time right?
If we used this helper function in a non-modal way would you get repeated popups?
There was a problem hiding this comment.
Hmm...since we're not using this for non-modals, I'll just remove the option.
And to be safe, I'll add an escape hatch where if someone clicks the "More Info", say, 5 times, the modal is closed and value returned is "no".
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Thanks for the detailed review of the tests. I'll address your comments.
extensions/ql-vscode/src/helpers.ts
Outdated
| if (chosenItem === urlItem) { | ||
| await env.openExternal(Uri.parse(url, true)); | ||
| } | ||
| } while (chosenItem === urlItem); |
There was a problem hiding this comment.
Hmm...since we're not using this for non-modals, I'll just remove the option.
And to be safe, I'll add an escape hatch where if someone clicks the "More Info", say, 5 times, the modal is closed and value returned is "no".
aeisenberg
force-pushed
the
aesienberg/show-info
branch
from
4a50ad1 to
50b107b
Compare
aeisenberg
force-pushed
the
aesienberg/show-info
branch
from
50b107b to
05d98d7
Compare
2 participants
This commit moves the URL in the text of the telemetry pop up to a
separate button. Clicking on the button will automatically open the
link in the browser.
Also, adds unit tests for the open dialog helper functions.
Checklist
@github/docs-content-dsphas been cc'd in all issues for UI or other user-facing changes made by this pull request.