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

Remove line about selecting a language from the dropdown. #957

Merged
merged 2 commits into from
Oct 1, 2021

Conversation

marcnjaramillo
Copy link
Contributor

@marcnjaramillo marcnjaramillo commented Sep 30, 2021

Closes #894

Changes made: removed the line about selecting a language from the dropdown, which now makes the download progress visible even when the popup is collapsed.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • @github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.

@marcnjaramillo marcnjaramillo requested a review from a team as a code owner September 30, 2021 21:16
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Nice, thanks for picking this up so quickly!
Some very small text suggestions, and a larger optional suggestion that would be interesting, but doesn't have to be in this PR.

extensions/ql-vscode/src/databases-ui.ts Outdated Show resolved Hide resolved
@@ -295,7 +295,7 @@ export class DatabaseUI extends DisposableObject {
'codeQLDatabases.chooseDatabaseLgtm',
this.handleChooseDatabaseLgtm,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional:
Here is a more involved suggestion for you to try out, which I think would work nicely. Consider it an option for further exploration: you can choose whether to include it as part of this PR, or complete this PR and try this suggestion in a follow-up PR.

If we look at the handleChooseDatabaseLgtm function, you can see it has a parameter progress: ProgressCallback. Progress callbacks work by allowing you to provide a progress message and step number, e.g. the code

progress({
  message: 'Choose project',
  step: 1,
  maxStep: 2
})

will update the progress notification box with the text title: message (e.g. "Adding database from LGTM: Choose project") and a 50% complete progress bar.

Instead of having the "Choose language" text in the title, which you've correctly removed in this PR, I think we could add two progress() calls -- one that says "Choose project" and one that says "Choose language". Then the popup box exactly describes the action we are asking the user to carry out in the UI at the top of the screen.

You may need to experiment to choose the best place to put those two progress calls. I think the function promptImportLgtmDatabase is a good place for the first one, and promptForLanguage is a good place for the second one. Both are reached from handleChooseDatabaseLgtm. The second one will require passing a progress parameter through to a few more functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea! I think for now I'll complete this PR so we can at least have some small improvement, and then I'll spend some time this afternoon and tomorrow working on it.

extensions/ql-vscode/src/extension.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Nice work! Could you also add a changelog message here? See how the other messages are written in the CHANGELOG.md file.

@marcnjaramillo
Copy link
Contributor Author

@aeisenberg Yes, I will do that. Sorry I missed it the first time.

@marcnjaramillo marcnjaramillo force-pushed the fix-lgtm-download-message branch from a180e62 to a5ba94b Compare October 1, 2021 18:04
@marcnjaramillo marcnjaramillo force-pushed the fix-lgtm-download-message branch from a5ba94b to b40f648 Compare October 1, 2021 18:08
@aeisenberg aeisenberg merged commit 39fdd0c into github:main Oct 1, 2021
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.

LGTM database download progress popup is misleading when not expanded
3 participants