Show DVC Cli Details in DVC Setup #3688
Conversation
| } from 'dvc/src/setup/webview/contract' | ||
| import { | ||
| MessageFromWebviewType, | ||
| MessageToWebview | ||
| } from 'dvc/src/webview/contract' | ||
| import React, { useCallback, useState } from 'react' | ||
| import { Dvc } from './Dvc' | ||
| import { Dvc } from './dvc/Dvc' |
There was a problem hiding this comment.
With DVC needing styles now, I moved DVC related components to a dvc folder.
|
Converting this to a draft for now since how we are going to display these details still needs work! |
| @@ -19,6 +20,9 @@ export const App: React.FC = () => { | |||
| const [cliCompatible, setCliCompatible] = useState<boolean | undefined>( | |||
| undefined | |||
| ) | |||
| const [dvcCliDetails, setDvcCliDetails] = useState<DvcCliDetails | undefined>( | |||
There was a problem hiding this comment.
Currently we're passing around an object, but we could also just have two new properties, dvcVersion and dvcExampleCommand.
There was a problem hiding this comment.
Both CliIncompatible and CliUnavailable have been simplified.
|
Design has been discussed in #3434 and I think we're ready for another round of reviews! |
| @@ -36,7 +40,6 @@ export const App: React.FC = () => { | |||
| const [sectionCollapsed, setSectionCollapsed] = useState( | |||
| DEFAULT_SECTION_COLLAPSED | |||
| ) | |||
|
|
|||
| const [isStudioConnected, setIsStudioConnected] = useState<boolean>(false) | |||
| const [shareLiveToStudio, setShareLiveToStudioValue] = | |||
| useState<boolean>(false) | |||
There was a problem hiding this comment.
As we're starting to have multiple levels of components and a rapidly growing state, it might be time to get Redux into that webview as well.
There was a problem hiding this comment.
Will update #3434!
| }) => { | ||
| const versionText = `${ | ||
| version || 'Not found' | ||
| } (required >= ${MIN_CLI_VERSION} and < ${MAX_CLI_VERSION}.0.0)` |
There was a problem hiding this comment.
I know I am going directly against this but I would but required between ${MIN_CLI_VERSION} and ${MAX_CLI_VERSION}.0.0 like
${MIN_CLI_VERSION} <= required < ${MAX_CLI_VERSION}.0.0
or
${MIN_CLI_VERSION} <= DVC < ${MAX_CLI_VERSION}.0.0
There was a problem hiding this comment.
Decided to go with ${MIN_CLI_VERSION} <= required < ${MAX_CLI_VERSION}.0.0
| <button className={styles.buttonAsLink} onClick={setupWorkspace}> | ||
| Configure | ||
| </button> | ||
| {isPythonExtensionInstalled && ( |
There was a problem hiding this comment.
[I] Bit of an edge case but if we are not using the python extension to "discover" the CLI then I don't think we want to display this as it can be confusing:
Screen.Recording.2023-04-26.at.12.13.27.pm.mov
This is why trying to jam everything into a single field is hard.
There was a problem hiding this comment.
Good catch! I'll only show it if the user is using the python extension or, if that's difficult to check, remove it entirely.
| @@ -8,6 +8,10 @@ import { App } from '../setup/components/App' | |||
| const DEFAULT_DATA: SetupData = { | |||
| canGitInitialize: false, | |||
| cliCompatible: true, | |||
| dvcCliDetails: { | |||
| exampleCommand: 'path/to/python -m dvc', | |||
There was a problem hiding this comment.
should this be invocation instead of exampleCommand? If not let's match it to the UI (command). Again, I know I am going directly against #3434 (comment) but I think we could find a better word in the UI for "Command".
There was a problem hiding this comment.
I'll update it to command for now!
julieg18
force-pushed
the
show-dvc-cli-details-in-setup
branch
from
11b8ea0 to
d9662cc
Compare
There was a problem hiding this comment.
Resolving #3688 (comment) ended up touching more code than I thought. Decided to comment on main changes before merging :)
| <button className={styles.buttonAsLink} onClick={setupWorkspace}> | ||
| Configure | ||
| </button> | ||
| {isPythonExtensionUsed && ( |
There was a problem hiding this comment.
With the changes to DVC setup, isPythonExtensionInstalled was only used here so I decided to replace isPythonExtensionInstalled with isPythonExtensionUsed.
| projectInitialized: false | ||
| }) | ||
|
|
||
| export const CliFoundManually = Template.bind({}) |
There was a problem hiding this comment.
Added a new story to show how things would look if python extension is not being used and replaced NoCLIPythonExtensionNotUsed and NoCLIPythonExtensionUsedwith NoCliPythonFound since the two previous stories look the same now.
|
Code Climate has analyzed commit a303201 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 94.7% (0.0% change). View more on Code Climate. |
Demo
For details on current design, see discussion in #3434
https://user-images.githubusercontent.com/43496356/234300243-444ba148-e626-450b-955a-bd28020cac78.movScreen.Recording.2023-04-27.at.1.22.54.PM.mov
Part of #3434