Move Connect to Studio into Setup webview #3452

mattseddon · 2023-03-13T05:21:11Z

2/4 `main` <- #3448 <- this <- #3462 <- #3463

Related to #3434.

This PR moves the Connect to Studio page inside of the Setup webview.

Demo

Screen.Recording.2023-03-13.at.7.36.18.pm.mov

webview/src/setup/components/App.tsx

codeclimate · 2023-03-13T05:28:58Z

webview/src/setup/components/Experiments.tsx

@@ -69,12 +69,16 @@ export const SetupExperiments: React.FC<SetupExperimentsProps> = ({
  }

  if (hasData === undefined) {
-    return <EmptyState>Loading Project...</EmptyState>
+    return <EmptyState isFullScreen={false}>Loading Project...</EmptyState>


Avoid too many return statements within this function.

Should we move this inside <NoData />? This would probably solve that codeclimate issue.

Did something about this in #3463.

codeclimate · 2023-03-13T05:28:58Z

webview/src/setup/components/Experiments.tsx

  }

  if (!hasData) {
    return <NoData />
  }

-  return <EmptyState>{"You're all setup"}</EmptyState>
+  return (


Avoid too many return statements within this function.

webview/src/setup/components/App.tsx

codeclimate · 2023-03-13T05:28:58Z

webview/src/setup/components/Experiments.tsx

@@ -26,7 +26,7 @@ export type SetupExperimentsProps = {
  pythonBinPath: string | undefined
 }

-export const SetupExperiments: React.FC<SetupExperimentsProps> = ({
+export const Experiments: React.FC<ExperimentsProps> = ({


Function Experiments has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2023-03-13T08:24:33Z

webview/src/setup/components/Experiments.tsx

+  }
+
+  if (!hasData) {
+    return <NoData />


Avoid too many return statements within this function.

codeclimate · 2023-03-13T08:24:33Z

webview/src/setup/components/Experiments.tsx

+    return <CliIncompatible checkCompatibility={checkCompatibility} />
+  }
+
+  if (cliCompatible === undefined) {


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

mattseddon · 2023-03-14T01:25:19Z

extension/package.json

@@ -399,11 +399,6 @@
        "command": "dvc.showCommands",
        "category": "DVC"
      },
-      {
-        "title": "Connect to Studio",


[F] I'll replace these in a follow-up.

The current behaviour is back UX but I will fix it in the next PR.

Screen.Recording.2023-03-14.at.12.29.42.pm.mov

The best idea at the moment seems to be to collapse all of the non-relevant sections.

mattseddon · 2023-03-14T01:27:17Z

extension/src/experiments/commands/index.ts

@@ -108,11 +108,11 @@ export const getShareExperimentAsCommitCommand =
  }

 export const getShareExperimentToStudioCommand =
-  (internalCommands: InternalCommands, connect: Connect) =>
+  (internalCommands: InternalCommands, setup: Setup) =>


[F] All of the externals of Connect have been moved directly onto Setup to reduce the size of this change. Will work out a smarter split once the dust settles.

mattseddon · 2023-03-14T01:34:22Z

extension/src/setup/webview/messages.ts

-    hasData: boolean | undefined
-  }) {
+    shareLiveToStudio
+  }: SetupData) {


Note to self: Consider using "Step" here instead of sending all of this data (consolidate logic).

mattseddon · 2023-03-14T01:35:30Z

extension/src/test/suite/setup/index.test.ts

@@ -633,5 +636,154 @@ suite('Setup Test Suite', () => {
        'should set dvc.cli.incompatible to false if the version is compatible'
      ).to.be.calledWithExactly('setContext', 'dvc.cli.incompatible', false)
    })
+
+    it('should handle a message from the webview to open Studio', async () => {


[F] These are the Connect test suite tests copied across.

mattseddon · 2023-03-14T01:36:50Z

webview/src/setup/components/App.test.tsx

-    render(<App />)
-    expect(mockPostMessage).toHaveBeenCalledWith({
-      type: MessageFromWebviewType.INITIALIZED
+  describe('Experiments', () => {


[F] Indentation change as per the new describe statement.

mattseddon · 2023-03-14T01:37:54Z

webview/src/setup/components/App.test.tsx

-      needsGitInitialized: undefined,
-      projectInitialized: false,
-      pythonBinPath: undefined
+  describe('Studio not connected', () => {


[F] Copy pasted from the old Connect App tests.

sroy3

Not sure why the section is not expanding

sroy3 · 2023-03-14T12:43:58Z

webview/src/setup/components/App.tsx

      ]
    )
  )

+  const setShareLiveToStudio = (shouldShareLive: boolean) => {
+    setShareLiveToStudioValue(shouldShareLive)
+    sendMessage({


Shouldn't this be inside messages.ts?

sroy3 · 2023-03-14T12:49:14Z

webview/src/setup/components/Experiments.tsx

@@ -26,7 +26,7 @@ export type SetupExperimentsProps = {
  pythonBinPath: string | undefined
 }

-export const SetupExperiments: React.FC<SetupExperimentsProps> = ({
+export const Experiments: React.FC<ExperimentsProps> = ({


I was wondering if a rename to Experiments was a good idea. After looking the experiments table main component is exported as ExperimentsTable (there is a <Experiments /> component, but it isn't exported). Just leaving this here in case anyone else was wondering. I'm fine with the renaming knowing these facts.

sroy3 · 2023-03-14T12:53:21Z

webview/src/setup/components/Experiments.tsx

@@ -69,12 +69,16 @@ export const SetupExperiments: React.FC<SetupExperimentsProps> = ({
  }

  if (hasData === undefined) {
-    return <EmptyState>Loading Project...</EmptyState>
+    return <EmptyState isFullScreen={false}>Loading Project...</EmptyState>


Should we move this inside <NoData />? This would probably solve that codeclimate issue.

sroy3 · 2023-03-14T13:00:53Z

Not sure why the section is not expanding

styles.emptySection has height: 33vh; as a styles. Maybe we should have a styles.flexibleHeight variant with height: auto instead or simply use something else than the empty section.

julieg18

Great work so far!

julieg18 · 2023-03-14T13:06:33Z

webview/src/setup/components/Experiments.tsx

-  return <EmptyState>{"You're all setup"}</EmptyState>
+  return (
+    <EmptyState isFullScreen={false}>
+      <h1>{"You're all setup"}</h1>


I don't think this needs curly braces:

Suggested change

<h1>{"You're all setup"}</h1>

<h1>You're all setup</h1>

Needs to be some variant of You're all setup without the braces which is why I opted for this. I changed this text in #3463 to Setup Complete. We should agree on what the message should actually be.

julieg18 · 2023-03-14T13:16:04Z

webview/src/shared/components/sectionContainer/SectionContainer.tsx

+      {"Configure the extension's connection to "}
+      <a href="https://studio.iterative.a">Studio</a>.<br />
+      Studio provides a collaboration platform for Machine Learning and is free
+      for small teams and individual contributors.


Spotted a couple minor things in the text:

first line is wrapped in curly braces

Should "Machine Learning" be capitalized?

link to studio is missing an "i"

Configure the extension's connection to{' '} <a href="https://studio.iterative.ai">Studio</a>.<br /> Studio provides a collaboration platform for machine learning and is free for small teams and individual contributors.

Text is adapted from:

link to studio is missing an "i"

That is why we have a const. I'll fix

If we care about the consistent use of {"'"} vs ' then we need to add a linter rule.

Only rule I have been able to find seems to be the opposite of what you're asking for: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-literals.md

Can add https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-curly-brace-presence.md though

If we care about the consistent use of {"'"} vs ' then we need to add a linter rule.

I've always gotten error warnings when it comes to apostrophes in the text and I've just corrected them without thinking about it.

But, apparently, you're not seeing them? weird 🤔

Only rule I have been able to find seems to be the opposite of what you're asking for: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-literals.md

Can add https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-curly-brace-presence.md though

I've alway's just assumed we don't want extra curly braces that aren't needed in the JSX. Personally, I find it harder to read. No strong preference though!

If we care about the consistent use of {"'"} vs ' then we need to add a linter rule.

I've always gotten error warnings when it comes to apostrophes in the text and I've just corrected them without thinking about it.

But, apparently, you're not seeing them? weird 🤔

Do you see those errors if there are braces wrapping the text?

mattseddon · 2023-03-14T19:30:54Z

Not sure why the section is not expanding

styles.emptySection has height: 33vh; as a styles. Maybe we should have a styles.flexibleHeight variant with height: auto instead or simply use something else than the empty section.

@sroy3 how about:

.emptySection {
  width: 100%;
  min-height: 33vh;
  height: auto;
}

This seems to work:

Screen.Recording.2023-03-15.at.6.29.26.am.mov

Screen.Recording.2023-03-15.at.6.28.32.am.mov

WDYT?

sroy3 · 2023-03-14T19:36:57Z

Not sure why the section is not expanding

styles.emptySection has height: 33vh; as a styles. Maybe we should have a styles.flexibleHeight variant with height: auto instead or simply use something else than the empty section.

@sroy3 how about:
.emptySection {
  width: 100%;
  min-height: 33vh;
  height: auto;
}
This seems to work:

WDYT?

I think this is the best solution

codeclimate · 2023-03-14T23:01:26Z

Code Climate has analyzed commit 375cdfe and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	5
Duplication	3

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

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

View more on Code Climate.

mattseddon added the product PR that affects product label Mar 13, 2023

mattseddon self-assigned this Mar 13, 2023

mattseddon changed the base branch from main to give-setup-section March 13, 2023 05:21

codeclimate bot reviewed Mar 13, 2023

View reviewed changes

mattseddon changed the base branch from give-setup-section to main March 13, 2023 08:16

codeclimate bot reviewed Mar 13, 2023

View reviewed changes

mattseddon commented Mar 14, 2023

View reviewed changes

mattseddon mentioned this pull request Mar 14, 2023

Give Setup webview a single section #3448

Merged

mattseddon commented Mar 14, 2023

View reviewed changes

mattseddon changed the base branch from main to give-setup-section March 14, 2023 01:42

mattseddon marked this pull request as ready for review March 14, 2023 01:45

mattseddon requested review from sroy3 and julieg18 as code owners March 14, 2023 01:45

mattseddon force-pushed the add-connect branch from c1591c6 to 514ea52 Compare March 14, 2023 01:46

mattseddon mentioned this pull request Mar 14, 2023

Provide option to open and focus relevant Setup section through UI #3462

Merged

mattseddon requested a review from shcheklein March 14, 2023 06:26

mattseddon mentioned this pull request Mar 14, 2023

Add show experiments button to final experiments screen #3463

Merged

sroy3 reviewed Mar 14, 2023

View reviewed changes

julieg18 reviewed Mar 14, 2023

View reviewed changes

sroy3 approved these changes Mar 14, 2023

View reviewed changes

Base automatically changed from give-setup-section to main March 14, 2023 22:53

move Connect to Studio into Setup webview

ac3fa82

mattseddon added 5 commits March 15, 2023 09:53

reduce duplication and refactor

9bd3aa8

fix test id of Studio tooltip

7037796

fix sending share live to studio config option

593d968

self review

a8f98f4

apply review feedback (fix massive bug)

375cdfe

mattseddon force-pushed the add-connect branch from b8d876e to 375cdfe Compare March 14, 2023 22:54

mattseddon enabled auto-merge (squash) March 14, 2023 22:55

mattseddon merged commit 5302a56 into main Mar 14, 2023

mattseddon deleted the add-connect branch March 14, 2023 23:09

mattseddon mentioned this pull request Mar 15, 2023

Add react/jsx-curly-brace-presence eslint rule #3471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Connect to Studio into Setup webview #3452

Move Connect to Studio into Setup webview #3452

mattseddon commented Mar 13, 2023 •

edited

Loading

codeclimate bot Mar 13, 2023

sroy3 Mar 14, 2023

mattseddon Mar 14, 2023

codeclimate bot Mar 13, 2023

codeclimate bot Mar 13, 2023

codeclimate bot Mar 13, 2023

codeclimate bot Mar 13, 2023

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023 •

edited

Loading

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023

sroy3 left a comment

sroy3 Mar 14, 2023

sroy3 Mar 14, 2023

sroy3 Mar 14, 2023

sroy3 commented Mar 14, 2023

julieg18 left a comment

julieg18 Mar 14, 2023

mattseddon Mar 14, 2023

julieg18 Mar 14, 2023

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023

mattseddon Mar 14, 2023

julieg18 Mar 15, 2023 •

edited

Loading

julieg18 Mar 15, 2023 •

edited

Loading

mattseddon Mar 15, 2023

mattseddon commented Mar 14, 2023 •

edited

Loading

sroy3 commented Mar 14, 2023

codeclimate bot commented Mar 14, 2023

Move Connect to Studio into Setup webview #3452

Move Connect to Studio into Setup webview #3452

Conversation

mattseddon commented Mar 13, 2023 • edited Loading

2/4 main <- #3448 <- this <- #3462 <- #3463

Demo

codeclimate bot Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Mar 13, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 13, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 13, 2023

Choose a reason for hiding this comment

codeclimate bot Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroy3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroy3 commented Mar 14, 2023

julieg18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

julieg18 Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon commented Mar 14, 2023 • edited Loading

sroy3 commented Mar 14, 2023

codeclimate bot commented Mar 14, 2023

mattseddon commented Mar 13, 2023 •

edited

Loading

2/4 `main` <- #3448 <- this <- #3462 <- #3463

mattseddon Mar 14, 2023 •

edited

Loading

julieg18 Mar 15, 2023 •

edited

Loading

julieg18 Mar 15, 2023 •

edited

Loading

mattseddon commented Mar 14, 2023 •

edited

Loading