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.

Sign up for GitHub

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

Jump to bottom

Consolidate Setup and "Get Started" walkthrough #3993

Merged
merged 11 commits into from
May 31, 2023
Merged

Consolidate Setup and "Get Started" walkthrough #3993

merged 11 commits into from
May 31, 2023

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented May 27, 2023 •
edited
Loading

1/2 main <= this <= #3994

  • keeps Setup focused on helping user setup DVC and the walkthrough focused on extension features

Demo

Screen.Recording.2023-05-30.at.12.18.29.PM.mov

https://github.com/iterative/vscode-dvc/assets/43496356/952dc855-5ae0-4c21-bb39-0a4dfa0662b3

https://github.com/iterative/vscode-dvc/assets/43496356/6ea6915f-15f7-4f26-9d2f-5b0ea84e2c8a

image image

Part of #3434

@julieg18 julieg18 added the product PR that affects product label May 27, 2023
@julieg18 julieg18 self-assigned this May 27, 2023
@julieg18 julieg18 mentioned this pull request May 27, 2023
Merged
julieg18
julieg18 commented May 30, 2023
View reviewed changes
webview/src/setup/components/dvc/CliUnavailable.tsx
@@ -33,7 +44,13 @@ export const CliUnavailable: React.FC<PropsWithChildren> = ({ children }) => {
<EmptyState isFullScreen={false}>
<h1>DVC is currently unavailable</h1>
{children}
{contents}
<p>
New to DVC? Check out <a href="https://dvc.org/">dvc.org</a> to learn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these DVC steps will now be the first thing that users new to DVC will see, I added some extra sentences/links that were once in the walkthrough.

@julieg18 julieg18 marked this pull request as ready for review May 30, 2023 17:31
@julieg18 julieg18 requested review from mattseddon and sroy3 as code owners May 30, 2023 17:31
@shcheklein
Copy link
Member

A few comments (might not be related to this PR, sorry if I missed some changes before, but since we have good screenshots here, it's worth mentioning). Not a blocker.

  • messages / screens became too verbose
  • all types if installation- is it true?
  • mentioning docs and demo before action items is misleading (they won't be able to try? Docs - we don't want people reading docs, only when it's needed for some advanced setup. Extension should be enough to do the setup.
  • in the initialize project- do we need both first sentences?

@julieg18
Copy link
Contributor Author

all types if installation- is it true?

We have global (dvc), manual (INPUTED_PATH -m dvc), and auto python environments. Looking at the docs, these cover all bases as far as I know.

in the initialize project- do we need both first sentences?

A bit out of scope, but will be easy enough to tweak these a bit!

Docs - we don't want people reading docs, only when it's needed for some advanced setup.

Makes sense! Will update the docs link.

mentioning docs and demo before action items is misleading

messages / screens became too verbose

Will give these some more thought and save for a followup 🤔

@shcheklein
Copy link
Member

these cover all bases as far as I know.

Ah, sorry. I read it more as "extension helps you to install it in all possible ways". Maybe we can rephrase the paragraph a bit.

sroy3
sroy3 approved these changes May 31, 2023
View reviewed changes
extension/resources/walkthrough/command-palette.md
> ℹ️ The extension's features cannot be accessed until DVC is installed and a
> DVC project is available in the workspace. Please refer to the
> [setup page](command:dvc.dvc.showDvcSetup) if you have not setup DVC yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be above the title?

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 added it at the top to better show that this admonition is for the walkthrough as a whole vs just this step. But if it looks buggy to you, I'm happy to move it.

Comparison of the two:

image image

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks and feels better after the title.

webview/src/setup/components/dvc/CliUnavailable.tsx Outdated Show resolved Hide resolved
@julieg18 julieg18 enabled auto-merge (squash) May 31, 2023 14:58
@codeclimate
Copy link

codeclimate bot commented May 31, 2023

Code Climate has analyzed commit 9750ed1 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.

@julieg18 julieg18 merged commit b68246b into main May 31, 2023
@julieg18 julieg18 deleted the update-get-started branch May 31, 2023 15:18
This was referenced May 31, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@sroy3 sroy3 sroy3 approved these changes

@mattseddon mattseddon mattseddon approved these changes

Assignees

@julieg18 julieg18

Labels
product PR that affects product
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@julieg18 @shcheklein @sroy3 @mattseddon