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

Use error foreground for status bar item color if CLI unavailable #2587

Merged
merged 3 commits into from
Oct 14, 2022

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Oct 13, 2022

From https://iterativeai.slack.com/archives/C01APS0FHDM/p1665690104423039?thread_ts=1665680827.468069&cid=C01APS0FHDM

"Extension is not initialized (should we be highlighting this better ( in red op yellow with a warning symbol?)"

Demo

Screen.Recording.2022-10-14.at.7.35.12.am.mov

@mattseddon mattseddon added the product PR that affects product label Oct 13, 2022
@mattseddon mattseddon self-assigned this Oct 13, 2022
@mattseddon mattseddon changed the title Make status bar item use error foreground if CLI unavailable Use error foreground for status bar item color if CLI unavailable Oct 13, 2022
@shcheklein
Copy link
Member

Cool, thanks Matt. Can we do the same on the side panel (where we put a message that DVC is incompatible). Also may be give a hint there about the current version of DVC, put a link to docs (get started walkthough)?

@mattseddon
Copy link
Member Author

Can we do the same on the side panel (where we put a message that DVC is incompatible).

I will check the API but I don't think so.

Also may be give a hint there about the current version of DVC, put a link to docs (get started walkthrough)?

There is already a toast popup with this information.

@shcheklein
Copy link
Member

There is already a toast popup with this information

Yes, but it's easy to dismiss it and then you are lost.

Another, out of scope, but a bit related suggestion to improve the awareness is to change the quick pick to always show three options (gray out the Python one if extension is not installed, show the message that it needs to be installed if you want to pick the env automatically)

@mattseddon
Copy link
Member Author

Yes, but it's easy to dismiss it and then you are lost.

Fair point but the text in the sidebar has to be static we cannot inject values into it.

Another, out of scope, but a bit related suggestion to improve the awareness is to change the quick pick to always show three options (gray out the Python one if extension is not installed, show the message that it needs to be installed if you want to pick the env automatically)

The API does not allow us to do this either.

@mattseddon mattseddon marked this pull request as ready for review October 14, 2022 05:07
@codeclimate
Copy link

codeclimate bot commented Oct 14, 2022

Code Climate has analyzed commit c7c4d46 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.8% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 58d8775 into main Oct 14, 2022
@mattseddon mattseddon deleted the make-status-bar-show-error branch October 14, 2022 18:38
@shcheklein
Copy link
Member

Thanks, Matt! It's a good improvement. We still need to think on what to do on the side panel / webviews to make it 100% clear what's going on and how to solve the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants