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

Point users to Python extension in DVC Setup #4124

Merged
merged 7 commits into from
Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions webview/src/setup/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ describe('App', () => {
/DVC & DVCLive cannot be auto-installed as Python was not located./
)
).toBeInTheDocument()
expect(screen.queryByText('Install')).not.toBeInTheDocument()
})

it('should tell the user they can auto-install DVC with a Python interpreter', () => {
Expand All @@ -169,7 +168,7 @@ describe('App', () => {
expect(screen.getByText('Install (pip)')).toBeInTheDocument()
})

it('should let the user find another Python interpreter to install DVC when the Python extension is not installed', () => {
it('should let the user locate DVC when the Python extension is not installed', () => {
renderApp({
cliCompatible: undefined,
dvcCliDetails: {
Expand All @@ -187,6 +186,28 @@ describe('App', () => {
})
})

it('should show "Set Env" and "Install (pip)" button tooltips when dvc is unavailable and Python extension is not installed', () => {
Copy link
Contributor Author

@julieg18 julieg18 Jun 17, 2023

Choose a reason for hiding this comment

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

My original plan was to add tests for checking if buttons and tooltips were disabled depending on circumstances but for some reason, tooltips and buttons weren't disabling in the tests. They are working in Storybook and in development but not in testing. Any ideas what's going on?

Copy link
Member

Choose a reason for hiding this comment

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

the magic virtual dom probably doesn't set the properly correctly

renderApp({
cliCompatible: undefined,
dvcCliDetails: {
command: 'python -m dvc',
version: undefined
}
})

const installButton = screen.getByText('Install (pip)')

fireEvent.mouseEnter(installButton, { bubbles: true })

expect(screen.getByTestId('install-tooltip')).toBeVisible()

const setEnvButton = screen.getByText('Set Env')

fireEvent.mouseEnter(setEnvButton, { bubbles: true })

expect(screen.getByTestId('set-env-tooltip')).toBeVisible()
})

it('should let the user find or create another Python interpreter to install DVC when the Python extension is installed', () => {
renderApp({
cliCompatible: undefined,
Expand Down
8 changes: 6 additions & 2 deletions webview/src/setup/components/dvc/CliIncompatible.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ export const CliIncompatible: React.FC<PropsWithChildren> = ({ children }) => {
const conditionalContents = canUpgrade ? (
<>
<div className={styles.sideBySideButtons}>
<Button onClick={upgradeDvc} text="Upgrade (pip)" />
<Button text="Check Compatibility" onClick={checkCompatibility} />
<span className={styles.buttonWrapper}>
<Button onClick={upgradeDvc} text="Upgrade (pip)" />
</span>
<span className={styles.buttonWrapper}>
<Button text="Check Compatibility" onClick={checkCompatibility} />
</span>
</div>
</>
) : (
Expand Down
69 changes: 59 additions & 10 deletions webview/src/setup/components/dvc/CliUnavailable.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { PropsWithChildren } from 'react'
import React, { PropsWithChildren, ReactElement } from 'react'
import { useSelector } from 'react-redux'
import styles from './styles.module.scss'
import { Button } from '../../../shared/components/button/Button'
Expand All @@ -10,6 +10,30 @@ import {
updatePythonEnvironment
} from '../../util/messages'
import { Warning } from '../../../shared/components/icons'
import { ExtensionLink } from '../shared/ExtensionLink'
import Tooltip from '../../../shared/components/tooltip/Tooltip'

const PythonExtensionTooltip: React.FC<{
dataTestId: string
disabled: boolean
children: ReactElement
}> = ({ dataTestId, disabled, children }) => (
<Tooltip
content={
<span data-testid={dataTestId}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we need the extra tooltips since it does tell you to install the python extension in the text 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the tooltip either. It looks very strange with almost the same text popping up next to the original.

Install the{' '}
<ExtensionLink extensionId="ms-python.python">
Python extension
</ExtensionLink>
.
</span>
}
interactive={true}
disabled={disabled}
>
<span>{children}</span>
</Tooltip>
)

export const CliUnavailable: React.FC<PropsWithChildren> = ({ children }) => {
const { pythonBinPath, isPythonExtensionUsed, isPythonEnvironmentGlobal } =
Expand All @@ -35,21 +59,17 @@ export const CliUnavailable: React.FC<PropsWithChildren> = ({ children }) => {
)}
.
</p>
<div className={styles.sideBySideButtons}>
<Button onClick={installDvc} text="Install (pip)" />
{isPythonExtensionUsed && (
<Button onClick={updatePythonEnvironment} text="Set Env" />
)}
<Button onClick={setupWorkspace} text="Locate DVC" />
</div>
</>
) : (
<>
<p>
{installationSentence} DVC & DVCLive cannot be auto-installed as Python
was not located.
was not located. Install the{' '}
<ExtensionLink extensionId="ms-python.python">
Python extension
</ExtensionLink>{' '}
to detect or create python environments.
</p>
<Button onClick={setupWorkspace} text="Locate DVC" />
</>
)

Expand All @@ -58,6 +78,35 @@ export const CliUnavailable: React.FC<PropsWithChildren> = ({ children }) => {
<h1>DVC is currently unavailable</h1>
{children}
{conditionalContents}
<div className={styles.sideBySideButtons}>
<span className={styles.buttonWrapper}>
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
<PythonExtensionTooltip
dataTestId="install-tooltip"
disabled={canInstall}
>
<Button
disabled={!canInstall}
onClick={installDvc}
text="Install (pip)"
/>
</PythonExtensionTooltip>
</span>
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
<span className={styles.buttonWrapper}>
<PythonExtensionTooltip
dataTestId="set-env-tooltip"
disabled={isPythonExtensionUsed}
>
<Button
disabled={!isPythonExtensionUsed}
onClick={updatePythonEnvironment}
text="Set Env"
/>
</PythonExtensionTooltip>
</span>
<span className={styles.buttonWrapper}>
<Button onClick={setupWorkspace} text="Locate DVC" />
</span>
</div>
</EmptyState>
)
}
6 changes: 5 additions & 1 deletion webview/src/setup/components/dvc/styles.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
font-size: inherit;
}

.sideBySideButtons > *:not(:first-child) {
.buttonWrapper {
display: inline-block;
}

.sideBySideButtons .buttonWrapper:not(:first-child) {
margin-left: 15px;
}
14 changes: 3 additions & 11 deletions webview/src/setup/components/remotes/ShowExtension.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ import React from 'react'
import styles from './styles.module.scss'
import { Icon } from '../../../shared/components/Icon'
import { Extensions } from '../../../shared/components/icons'
import { ExtensionLink } from '../shared/ExtensionLink'
Copy link
Member

Choose a reason for hiding this comment

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

[Q] Why not move the text out from the original paragraph, put it under the buttons and use this component?

I think that would

  1. draw attention to the text
  2. remove the need to show a tooltip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Here's what that looks like so far:

Screenshot 2023-06-19 at 11 01 48 AM

I adjusted the paragraph styling so that it looks more connected to the disabled buttons vs a separate paragraph entirely.


export const ShowExtension: React.FC<{
capabilities: string
id: string
name: string
}> = ({ capabilities, id, name }) => {
const idQuery = `"@id:${id}"`

return (
<p>
<Icon
Expand All @@ -18,15 +17,8 @@ export const ShowExtension: React.FC<{
height={16}
className={styles.infoIcon}
/>{' '}
The{' '}
<a
href={`command:workbench.extensions.search?${encodeURIComponent(
idQuery
)}`}
>
{name}
</a>{' '}
extension can be used to <span>{capabilities}</span>.
The <ExtensionLink extensionId={id}>{name}</ExtensionLink> extension can
be used to <span>{capabilities}</span>.
</p>
)
}
24 changes: 24 additions & 0 deletions webview/src/setup/components/shared/ExtensionLink.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React, { PropsWithChildren } from 'react'
import type { HTMLAttributes } from 'react'

interface ExtensionLinkProps extends HTMLAttributes<HTMLAnchorElement> {
extensionId: string
}

export const ExtensionLink: React.FC<PropsWithChildren<ExtensionLinkProps>> = ({
extensionId,
children,
...props
}) => {
const idQuery = `"@id:${extensionId}"`
return (
<a
href={`command:workbench.extensions.search?${encodeURIComponent(
idQuery
)}`}
{...props}
>
{children}
</a>
)
}
1 change: 1 addition & 0 deletions webview/src/shared/components/button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export type ButtonProps = {
isNested?: boolean
children?: React.ReactNode
disabled?: boolean
className?: string
}

export const Button: React.FC<ButtonProps> = ({
Expand Down