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

Add prompt to show/install the Red Hat YAML extension #903

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Oct 13, 2021

2/2 master <- #902 <- this

As discussed in the planning meeting this morning we don't want to have an explicit dependency on the Red Hat YAML extension.

The best way around that I have found is to show the user the specific extension and let them install it if they choose to.

Demo

Screen.Recording.2021-10-13.at.2.33.09.pm.mov

Thanks to @skshetry for the assist with simplifying the code.

@mattseddon mattseddon added the product PR that affects product label Oct 13, 2021
@mattseddon mattseddon self-assigned this Oct 13, 2021
@@ -10,3 +10,13 @@ export const getYesOrNoOrNever = (
Response.NO,
Response.NEVER
)

export const getShowOrCloseOrNever = (
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

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

🥇

@mattseddon mattseddon changed the base branch from master to rename-language-association October 13, 2021 04:23
@mattseddon mattseddon marked this pull request as ready for review October 13, 2021 04:24
const response = await getShowOrCloseOrNever(
'It is recommended that you install the ' +
'[Red Hat YAML](https://marketplace.visualstudio.com/items?itemName=redhat.vscode-yaml) ' +
'extension for comprehensive YAML Language support and [DVC YAML schema validation](https://github.com/iterative/dvcyaml-schema).'
Copy link
Member Author

@mattseddon mattseddon Oct 13, 2021

Choose a reason for hiding this comment

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

[F] Both links are shown in the demo. Happy to take help with copy.

@mattseddon mattseddon changed the title Add prompt for installing the Red Hat YAML extension Add prompt to show/install the Red Hat YAML extension Oct 13, 2021
@mattseddon
Copy link
Member Author

Relates to #46.

@@ -49,6 +49,8 @@
"command.views.experimentsSortByTree.removeSort": "Remove Sort From Experiments Table",
"config.doNotAssociateYaml.description": "Do not ask to add records into the user's settings relating to associating .dvc and dvc.lock files with the YAML data serialization language",
"config.doNotAssociateYaml.title": "Do not ask to associate .dvc and dvc.lock files with YAML.",
"config.doNotRecommendRedHatExtension.description": "Do not prompt the user to install the Red Hat YAML extension to assist with DVC YAML schema validation.",
"config.doNotRecommendRedHatExtension.title": "Do not prompt the user to install the Red Hat YAML extension.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I mistaken to think that these strings are shown to the user somewhere? (the settings maybe?) If so, I would drop the "the user" from both. If these are simply used internally, then never mind my comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're correct that they're in the settings:

image

I've updated

Copy link
Member Author

Choose a reason for hiding this comment

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

And thank you

Base automatically changed from rename-language-association to master October 13, 2021 19:13
@codeclimate
Copy link

codeclimate bot commented Oct 13, 2021

Code Climate has analyzed commit 808bf0d and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

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

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

View more on Code Climate.

@mattseddon mattseddon merged commit 3dd1cd1 into master Oct 13, 2021
@mattseddon mattseddon deleted the add-red-hat-recommendation branch October 13, 2021 19:52
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.

2 participants