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 link to show applicable extension for each storage provider #4004

Merged
merged 1 commit into from
May 31, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented May 30, 2023

4/4 main <- #4005 <- #4006 <- #4007 <- this

Related to #3867

This PR adds some clickable links to tabs in the<StorageSlider/> added in #3996. Each link will show the appropriate extension for the storage provider in the extensions view.

Demo

Screen.Recording.2023-05-30.at.1.58.39.pm.mov

@mattseddon mattseddon added the product PR that affects product label May 30, 2023
@mattseddon mattseddon self-assigned this May 30, 2023
<a
href={`command:workbench.extensions.search?${encodeURIComponent(
idQuery
)}`}
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] This took a long time to workout. I actually had to visit the VS Code codebase to copy the expected format.

Finally found the format at
https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/welcomeGettingStarted/common/media/extensions-web.svg#L44

@mattseddon mattseddon marked this pull request as ready for review May 30, 2023 02:21
@mattseddon mattseddon requested review from sroy3 and julieg18 as code owners May 30, 2023 02:21
@mattseddon mattseddon requested a review from shcheklein May 30, 2023 02:41
@mattseddon mattseddon force-pushed the add-show-extension branch from 51e112e to c3f0393 Compare May 30, 2023 03:59
@mattseddon mattseddon changed the base branch from main to fix-style May 30, 2023 03:59
@mattseddon
Copy link
Member Author

cc @dberenbaum for the demo. It shows the information currently provided per supported storage type.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work! Had some minor comments about the content :)

@@ -11,6 +12,10 @@ export const AzureBlobStorage = () => (
</a>{' '}
for details on how to authenticate.
</p>
<ShowExtension
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's just me as a pretty literal person, but a sentence telling me about the separate extension felt confusing. Do I need this extension to setup DVC remotes? Looking at the comment you linked in #3867, I'm assuming the purpose is to let the user know that installing the corresponding remote extension would make remote setup easier 🤔

Maybe we could make that reasoning more clear? For example, Make setup easier with the Azure Storage extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we could make that reasoning more clear? For example, Make setup easier with the Azure Storage extension.

Without doing some due diligence I wouldn't make that claim. I am going to see if those extensions expose useful APIs but for now I just wanted to make people aware that they exist.


return (
<p>
<Icon
Copy link
Contributor

Choose a reason for hiding this comment

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

With this admonition added, there are now 2-3 admonitions stacked up on top of each other in each panel:

image

This feels hard to read for me but I'm not really seeing an alternative solution besides rewording these into paragraphs instead of admonitions which might not actually make these more readable 🤔

@mattseddon mattseddon force-pushed the add-show-extension branch from c3f0393 to 58d2205 Compare May 30, 2023 22:16
Base automatically changed from fix-style to main May 30, 2023 23:18
@mattseddon mattseddon force-pushed the add-show-extension branch from 58d2205 to 24c0b77 Compare May 30, 2023 23:19
@mattseddon mattseddon enabled auto-merge (squash) May 30, 2023 23:19
@codeclimate
Copy link

codeclimate bot commented May 30, 2023

Code Climate has analyzed commit 24c0b77 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 95.2% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit f48cce8 into main May 31, 2023
@mattseddon mattseddon deleted the add-show-extension branch May 31, 2023 01:11
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.

3 participants