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 reset number of commits to show for a branch button to experiments table #4355

Merged
merged 1 commit into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions extension/src/experiments/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,11 @@ export class ExperimentsModel extends ModelWithPersistence {
this.persistNbOfCommitsToShow()
}

public resetNbfCommitsToShow(branch: string) {
delete this.numberOfCommitsToShow[branch]
Copy link
Contributor

Choose a reason for hiding this comment

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

I've looked over the code multiple times but I can't figure out how this change is saved in the state across sessions. The function only touches this.numberOfCommitsToShow yet if I reload the window, the change is saved so the numberOfCommitsToShow is being persisted somewhere, right? How is it working though 😅 What am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

The memento could just be holding a reference to the original object. I will make an update.

this.persistNbOfCommitsToShow()
}

public getNbOfCommitsToShow(branch: string) {
return (
this.numberOfCommitsToShow[branch] ||
Expand Down
24 changes: 18 additions & 6 deletions extension/src/experiments/webview/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,11 @@ export class WebviewMessages {

case MessageFromWebviewType.SHOW_MORE_COMMITS:
return this.changeCommitsToShow(1, message.payload)

case MessageFromWebviewType.SHOW_LESS_COMMITS:
return this.changeCommitsToShow(-1, message.payload)
case MessageFromWebviewType.RESET_COMMITS:
return this.resetCommitsToShow(message.payload)

case MessageFromWebviewType.SELECT_BRANCHES:
return this.addAndRemoveBranches()

Expand Down Expand Up @@ -221,19 +223,29 @@ export class WebviewMessages {
}

private async changeCommitsToShow(change: 1 | -1, branch: string) {
this.experiments.setNbfCommitsToShow(
this.experiments.getNbOfCommitsToShow(branch) +
NUM_OF_COMMITS_TO_INCREASE * change,
branch
)
await this.update()
sendTelemetryEvent(
change === 1
? EventName.VIEWS_EXPERIMENTS_TABLE_SHOW_MORE_COMMITS
: EventName.VIEWS_EXPERIMENTS_TABLE_SHOW_LESS_COMMITS,
undefined,
undefined
)
this.experiments.setNbfCommitsToShow(
this.experiments.getNbOfCommitsToShow(branch) +
NUM_OF_COMMITS_TO_INCREASE * change,
branch
)
}

private async resetCommitsToShow(branch: string) {
this.experiments.resetNbfCommitsToShow(branch)
await this.update()
sendTelemetryEvent(
EventName.VIEWS_EXPERIMENTS_TABLE_RESET_COMMITS,
undefined,
undefined
)
}

private refreshData() {
Expand Down
3 changes: 3 additions & 0 deletions extension/src/telemetry/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ export const EventName = Object.assign(
VIEWS_EXPERIMENTS_TABLE_REFRESH: 'views.experimentsTable.refresh',
VIEWS_EXPERIMENTS_TABLE_REMOVE_COLUMN_SORT:
'views.experimentsTable.columnSortRemoved',
VIEWS_EXPERIMENTS_TABLE_RESET_COMMITS:
'views.experimentsTable.resetCommits',
VIEWS_EXPERIMENTS_TABLE_RESIZE_COLUMN:
'views.experimentsTable.columnResized',
VIEWS_EXPERIMENTS_TABLE_SELECT_BRANCHES:
Expand Down Expand Up @@ -246,6 +248,7 @@ export interface IEventNamePropertyMapping {
[EventName.VIEWS_EXPERIMENTS_TABLE_REMOVE_COLUMN_SORT]: {
path: string
}
[EventName.VIEWS_EXPERIMENTS_TABLE_RESET_COMMITS]: undefined
[EventName.VIEWS_EXPERIMENTS_TABLE_CREATED]: undefined
[EventName.VIEWS_EXPERIMENTS_TABLE_FOCUS_CHANGED]: WebviewFocusChangedProperties
[EventName.VIEWS_EXPERIMENTS_TABLE_HIDE_COLUMN_PATH]: {
Expand Down
33 changes: 33 additions & 0 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1480,6 +1480,39 @@ suite('Experiments Test Suite', () => {
expect(setNbfCommitsToShowSpy).to.be.calledWith(3, 'main')
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should handle a message to reset the number of commits shown for a branch', async () => {
const {
experiments,
experimentsModel,
messageSpy,
mockUpdateExperimentsData
} = setupExperimentsAndMockCommands()

experimentsModel.setNbfCommitsToShow(100, 'main')
const getNumberOfCommitsToShowForMain = () =>
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(experimentsModel as any).numberOfCommitsToShow.main
expect(getNumberOfCommitsToShowForMain()).to.equal(100)

const resetNbfCommitsToShowSpy = spy(
experimentsModel,
'resetNbfCommitsToShow'
)

const webview = await experiments.showWebview()
messageSpy.resetHistory()
const mockMessageReceived = getMessageReceivedEmitter(webview)

mockMessageReceived.fire({
payload: 'main',
type: MessageFromWebviewType.RESET_COMMITS
})

expect(mockUpdateExperimentsData).to.be.calledOnce
expect(resetNbfCommitsToShowSpy).to.be.calledWithExactly('main')
expect(getNumberOfCommitsToShowForMain()).to.be.undefined
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should handle a message to select branches', async () => {
const {
experiments,
Expand Down
5 changes: 5 additions & 0 deletions extension/src/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export enum MessageFromWebviewType {
REORDER_PLOTS_TEMPLATES = 'reorder-plots-templates',
REFRESH_EXP_DATA = 'refresh-exp-data',
REFRESH_REVISIONS = 'refresh-revisions',
RESET_COMMITS = 'reset-commits',
RESIZE_COLUMN = 'resize-column',
RESIZE_PLOTS = 'resize-plots',
SAVE_STUDIO_TOKEN = 'save-studio-token',
Expand Down Expand Up @@ -119,6 +120,10 @@ export type MessageFromWebview =
type: MessageFromWebviewType.REORDER_COLUMNS
payload: string[]
}
| {
type: MessageFromWebviewType.RESET_COMMITS
payload: string
}
| {
type: MessageFromWebviewType.RESIZE_COLUMN
payload: ColumnResizePayload
Expand Down
1 change: 1 addition & 0 deletions webview/icons/codicons.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export const codicons = [
'cloud-upload',
'cloud',
'copy',
'discard',
'ellipsis',
'error',
'extensions',
Expand Down
21 changes: 20 additions & 1 deletion webview/src/experiments/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1868,7 +1868,7 @@ describe('App', () => {
})
})

describe('Show more commits', () => {
describe('Change number of commits', () => {
it('should display a show more commits button', () => {
renderTable({ ...tableDataFixture, hasMoreCommits: { main: true } })

Expand Down Expand Up @@ -1933,6 +1933,25 @@ describe('App', () => {
type: MessageFromWebviewType.SHOW_LESS_COMMITS
})
})

it('should display a reset commits button', () => {
renderTable({ ...tableDataFixture, hasMoreCommits: { main: true } })

expect(
screen.getByLabelText('Reset Commits to Default')
).toBeInTheDocument()
})

it('should send a message to reset commits when the reset commits button is clicked', () => {
renderTable()

fireEvent.click(screen.getByLabelText('Reset Commits to Default'))

expect(mockPostMessage).toHaveBeenCalledWith({
payload: 'main',
type: MessageFromWebviewType.RESET_COMMITS
})
})
})

describe('Add / Remove branches', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,25 @@ import styles from './styles.module.scss'
import { Icon, IconValue } from '../../../../../shared/components/Icon'
import Tooltip from '../../../../../shared/components/tooltip/Tooltip'
export interface CommitsButtonProps {
icon: IconValue
moreOrLess: 'More' | 'Less'
action: () => void
disabled: boolean
icon: IconValue
tooltipContent: string
}

export const CommitsButton: React.FC<CommitsButtonProps> = ({
icon,
moreOrLess,
action,
disabled
disabled,
icon,
tooltipContent
}) => {
const text = `Show ${moreOrLess} Commits`
return (
<Tooltip content={<>{text}</>}>
<Tooltip content={tooltipContent}>
<button
className={styles.commitsButton}
onClick={action}
disabled={disabled}
aria-label={text}
aria-label={tooltipContent}
>
<Icon icon={icon} className={styles.commitsIcon} />
</button>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@ import React from 'react'
import { useSelector } from 'react-redux'
import styles from './styles.module.scss'
import { CommitsButton, CommitsButtonProps } from './CommitsButton'
import { showLessCommits, showMoreCommits } from '../../../../util/messages'
import {
resetCommits,
showLessCommits,
showMoreCommits
} from '../../../../util/messages'
import { ExperimentsState } from '../../../../store'
import { Add, Remove } from '../../../../../shared/components/icons'
import { Add, Discard, Remove } from '../../../../../shared/components/icons'
interface CommitsNavigationProps {
branch: string
}
Expand All @@ -16,26 +20,41 @@ export const CommitsNavigation: React.FC<CommitsNavigationProps> = ({
(state: ExperimentsState) => state.tableData
)

const commitsButtons: CommitsButtonProps[] = [
const getMoreOrLessValues = (
moreOrLess: 'More' | 'Less'
): { key: string; tooltipContent: string } => ({
key: moreOrLess,
tooltipContent: `Show ${moreOrLess} Commits`
})

const commitsButtons: (CommitsButtonProps & { key: string })[] = [
{
action: () => showMoreCommits(branch),
disabled: !hasMoreCommits[branch],
icon: Add,
moreOrLess: 'More'
...getMoreOrLessValues('More')
},
{
action: () => showLessCommits(branch),
disabled: !isShowingMoreCommits[branch],
icon: Remove,
moreOrLess: 'Less'
...getMoreOrLessValues('Less')
},
{
action: () => resetCommits(branch),
disabled: false,
icon: Discard,
key: 'Reset',
tooltipContent: 'Reset Commits to Default'
}
]

return (
<div className={styles.commitsNav}>
{commitsButtons.map(commitButton => (
<CommitsButton key={commitButton.moreOrLess} {...commitButton} />
))}
{commitsButtons.map(commitButton => {
const { key, ...commitButtonProps } = commitButton
return <CommitsButton key={key} {...commitButtonProps} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the index provided by the map function as the key value? Then we could remove the key pair from the commitsButton arr.

})}
</div>
)
}
6 changes: 6 additions & 0 deletions webview/src/experiments/util/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ export const reorderColumns = (newOrder: string[]) =>
type: MessageFromWebviewType.REORDER_COLUMNS
})

export const resetCommits = (branch: string) =>
sendMessage({
payload: branch,
type: MessageFromWebviewType.RESET_COMMITS
})

export const resizeColumn = (id: string, width: number) =>
sendMessage({
payload: { id, width },
Expand Down
19 changes: 19 additions & 0 deletions webview/src/shared/components/icons/Discard.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import * as React from 'react'
import type { SVGProps } from 'react'
const Discard = (props: SVGProps<SVGSVGElement>) => (
<svg
width={16}
height={16}
viewBox="0 0 16 16"
xmlns="http://www.w3.org/2000/svg"
fill="currentColor"
{...props}
>
<path
fillRule="evenodd"
clipRule="evenodd"
d="M3.5 2v3.5L4 6h3.5V5H4.979l.941-.941a3.552 3.552 0 1 1 5.023 5.023L5.746 14.28l.72.72 5.198-5.198A4.57 4.57 0 0 0 5.2 3.339l-.7.7V2h-1z"
/>
</svg>
)
export default Discard
1 change: 1 addition & 0 deletions webview/src/shared/components/icons/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export { default as Close } from './Close'
export { default as Cloud } from './Cloud'
export { default as CloudUpload } from './CloudUpload'
export { default as Copy } from './Copy'
export { default as Discard } from './Discard'
export { default as Ellipsis } from './Ellipsis'
export { default as Error } from './Error'
export { default as Extensions } from './Extensions'
Expand Down
Loading