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 CLI incompatible screen to Studio section #3763

Merged
merged 3 commits into from
Apr 26, 2023
Merged

Add CLI incompatible screen to Studio section #3763

merged 3 commits into from
Apr 26, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Apr 26, 2023

We are moving to using dvc config to store the Studio token. This PR fixes an upcoming edge case where the user would be able to attempt to connect to Studio without having access to the CLI.

Demo

Screen.Recording.2023-04-26.at.4.06.53.pm.mov

Note: I have also split grouped the Studio setup section into its own folder and split out each of the components into their own file.

@mattseddon mattseddon added the product PR that affects product label Apr 26, 2023
@mattseddon mattseddon self-assigned this Apr 26, 2023
import { EmptyState } from '../../../shared/components/emptyState/EmptyState'
import { Button } from '../../../shared/components/button/Button'

export const Connect: React.FC = () => {
Copy link

Choose a reason for hiding this comment

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

Function Connect has 41 lines of code (exceeds 40 allowed). Consider refactoring.

@mattseddon mattseddon marked this pull request as ready for review April 26, 2023 06:14
@@ -0,0 +1,25 @@
import React from 'react'
Copy link
Member Author

Choose a reason for hiding this comment

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

Feels like we're trying to head in the direction of splitting out components. LMK if I got this wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to help greatly in keeping styles simple and easy to clean out. It'll also help with Codeclimate issues and even tests.

@@ -0,0 +1,25 @@
import React from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to help greatly in keeping styles simple and easy to clean out. It'll also help with Codeclimate issues and even tests.

onClick={saveStudioToken}
/>
<p>
{"Don't Have an account?\n"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding a line break, I would add a display: block; on the following link. (It's about content vs. style).

Copy link
Member Author

Choose a reason for hiding this comment

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

this line break didn't even work. I have removed

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.

Looks good!

@mattseddon mattseddon enabled auto-merge (squash) April 26, 2023 21:46
@mattseddon mattseddon disabled auto-merge April 26, 2023 21:46
@codeclimate
Copy link

codeclimate bot commented Apr 26, 2023

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

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

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

View more on Code Climate.

@mattseddon mattseddon enabled auto-merge (squash) April 26, 2023 21:54
@mattseddon mattseddon merged commit 626328a into main Apr 26, 2023
@mattseddon mattseddon deleted the split-studio branch April 26, 2023 21:58
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