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

Remove click listener from experiment name and sha #4543

Merged
merged 2 commits into from
Aug 21, 2023
Merged

Remove click listener from experiment name and sha #4543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f167d50
Remove click listener from experiment name and sha
mattseddon Aug 20, 2023
c8fd82a
Merge branch 'main' into fix-4534
mattseddon Aug 21, 2023
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
71 changes: 14 additions & 57 deletions webview/src/experiments/components/App.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,12 @@ describe('App', () => {
})

describe('Toggle experiment status', () => {
it('should send a message to the extension to toggle an experiment when the row is clicked', () => {
it('should send a message to the extension to toggle an experiment when a plot icon is clicked', () => {
renderTable()

const testClick = (element: HTMLElement, id: string) => {
const testClick = (elementId: string, id: string) => {
const element = within(getRow(elementId)).getByTestId('plot-icon')

mockPostMessage.mockReset()

fireEvent.click(element)
Expand All @@ -339,56 +341,10 @@ describe('App', () => {
})
}

const [workspace, experimentRunningInWorkspace] = screen.getAllByText(
EXPERIMENT_WORKSPACE_ID
)

testClick(workspace, EXPERIMENT_WORKSPACE_ID)
testClick(experimentRunningInWorkspace, 'exp-83425')
testClick(screen.getAllByText('main')[1], 'main')
testClick(screen.getByText('[exp-e7a67]'), 'exp-e7a67')
})

it('should send a message to the extension to toggle an experiment when Enter or Space is pressed on the row', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] I spent 20-30 minutes trying to fix this test but could not come up with anything that worked. The behaviour does work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding this feature, but when I tabbed onto a table row and pressed enter/space nothing was toggled 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Focus needs to be on the plot icon. I'll record a demo later

Copy link
Member Author

Choose a reason for hiding this comment

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

Screen.Recording.2023-08-22.at.8.00.05.am.mov

renderTable()

mockPostMessage.mockClear()

const testRowLabel = screen.getAllByText('main')[1]

testRowLabel.focus()

fireEvent.keyDown(testRowLabel, {
bubbles: true,
code: 'Enter',
key: 'Enter',
keyCode: 13
})
expect(mockPostMessage).toHaveBeenCalledWith({
payload: 'main',
type: MessageFromWebviewType.TOGGLE_EXPERIMENT
})
mockPostMessage.mockClear()

fireEvent.keyDown(testRowLabel, {
bubbles: true,
charCode: 32,
code: 'Space',
key: ' ',
keyCode: 32
})
expect(mockPostMessage).toHaveBeenCalledWith({
payload: 'main',
type: MessageFromWebviewType.TOGGLE_EXPERIMENT
})
mockPostMessage.mockClear()

fireEvent.keyDown(testRowLabel, {
bubbles: true,
code: 'keyA',
key: 'a'
})
expect(mockPostMessage).not.toHaveBeenCalled()
testClick(EXPERIMENT_WORKSPACE_ID, EXPERIMENT_WORKSPACE_ID)
testClick('[exp-83425]', 'exp-83425')
testClick('main', 'main')
testClick('[exp-e7a67]', 'exp-e7a67')
})

it('should not send a message if row label was selected', () => {
Expand All @@ -412,13 +368,14 @@ describe('App', () => {
clearSelection()
fireEvent.click(getWorkspace())

expect(mockPostMessage).toHaveBeenCalledTimes(1)
expect(mockPostMessage).toHaveBeenCalledWith({
expect(mockPostMessage).not.toHaveBeenCalledTimes(1)
expect(mockPostMessage).not.toHaveBeenCalledWith({
payload: testRowId,
type: MessageFromWebviewType.TOGGLE_EXPERIMENT
})
})
it('should send a message if some other label is selected', () => {

it('should not send a message if some other label is selected', () => {
renderTable()
mockPostMessage.mockClear()

Expand All @@ -428,8 +385,8 @@ describe('App', () => {
createWindowTextSelection(selectedTestRowId, 5)
fireEvent.click(screen.getAllByText(testRowId)[1])

expect(mockPostMessage).toHaveBeenCalledTimes(1)
expect(mockPostMessage).toHaveBeenCalledWith({
expect(mockPostMessage).not.toHaveBeenCalledTimes(1)
expect(mockPostMessage).not.toHaveBeenCalledWith({
payload: testRowId,
type: MessageFromWebviewType.TOGGLE_EXPERIMENT
})
Expand Down
16 changes: 1 addition & 15 deletions webview/src/experiments/components/table/body/Cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { ExperimentStatusIndicator } from './ExperimentStatusIndicator'
import styles from '../styles.module.scss'
import { CellValue, isValueWithChanges } from '../content/Cell'
import { CellProp, RowProp } from '../../../util/interfaces'
import { clickAndEnterProps } from '../../../../util/props'
import { ErrorTooltip } from '../../../../shared/components/tooltip/ErrorTooltip'

const cellHasChanges = (cellValue: CellValue) =>
Expand Down Expand Up @@ -51,16 +50,8 @@ export const StubCell: React.FC<
}
} = cell
const {
original: {
error,
executorStatus,
gitRemoteStatus,
label,
id,
description = ''
}
original: { error, executorStatus, gitRemoteStatus, id }
} = row
const { toggleExperiment } = rowActionsProps

return (
<td className={cx(styles.experimentsTd, styles.experimentCell)}>
Expand All @@ -80,11 +71,6 @@ export const StubCell: React.FC<
[styles.workspaceChangeText]: changesIfWorkspace,
[styles.errorText]: error
})}
{...clickAndEnterProps(
toggleExperiment,
[label, description],
true
)}
data-testid={`id___${id}`}
>
{flexRender(columnCell, getContext())}
Expand Down
1 change: 1 addition & 0 deletions webview/src/experiments/components/table/body/CellRowActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export const CellRowActions: React.FC<CellRowActionsProps> = ({
>
<Icon
className={styles.plotBox}
data-testid="plot-icon"
style={
plotColor
? {
Expand Down