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

Show "Custom" section in "Get Started" screen #3523

Merged
merged 15 commits into from
Apr 3, 2023
Merged

Show "Custom" section in "Get Started" screen #3523

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
883737d
Show custom plots in Get Started screen
julieg18 Mar 21, 2023
e1c7dbf
Merge branch 'main' into show-custom-plots-data-in-get-started
julieg18 Mar 24, 2023
222b9b6
Fix bugs
julieg18 Mar 25, 2023
547fbcb
Remove ribbon
julieg18 Mar 27, 2023
fddccd0
Fix typo
julieg18 Mar 27, 2023
d498e0c
Cleanup
julieg18 Mar 27, 2023
5619a0c
Merge branch 'main' into show-custom-plots-data-in-get-started
julieg18 Mar 27, 2023
7c90fbe
Merge branch 'main' into show-custom-plots-data-in-get-started
julieg18 Mar 28, 2023
6ed0863
Use message in section instead of empty plots
julieg18 Mar 28, 2023
8ca7c73
Remove trends logic
julieg18 Mar 28, 2023
3bf96bd
Only show custom section when there are plots
julieg18 Mar 31, 2023
7374c2e
Merge branch 'main' into show-custom-plots-data-in-get-started
mattseddon Apr 3, 2023
771b9e0
Use only one text sentence
julieg18 Apr 3, 2023
a2763b4
Merge branch 'main' into show-custom-plots-data-in-get-started
julieg18 Apr 3, 2023
39b349a
Merge branch 'main' into show-custom-plots-data-in-get-started
julieg18 Apr 3, 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
7 changes: 4 additions & 3 deletions extension/src/plots/model/collect.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,10 @@ describe('collectCustomPlots', () => {
expect(data).toStrictEqual(expectedOutput)
})

it('should return only custom plots if there no selected revisions', () => {
const expectedOutput: CustomPlotData[] = customPlotsFixture.plots.filter(
plot => plot.type !== CustomPlotType.CHECKPOINT
it('should return checkpoint plots with empty if there no selected revisions', () => {
const expectedOutput: CustomPlotData[] = customPlotsFixture.plots.map(
plot =>
plot.type === CustomPlotType.CHECKPOINT ? { ...plot, values: [] } : plot
Copy link
Member

Choose a reason for hiding this comment

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

This feels odd:

image

I don't know

  1. Which plot is which
  2. What type of plot should be inside (trends or metrics vs params).

If there are no experiments at all then we end up with this:

image

Seems inconsistent.

At the very least I think the text inside of the empty box needs to mention the plot title/select a checkpoint experiment to display but I think they should be hidden with some message to say "select a checkpoint experiment to display trend plots"

Note: Chances of users seeing these screens are minimal due to the use of checkpoint experiments but I think we should attempt to get it right.

Copy link
Member

Choose a reason for hiding this comment

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

Also, need to consider when there are no custom plots added but we do have data available:

image

An update will definitely be needed after #3567. Probably want to add a button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the very least I think the text inside of the empty box needs to mention the plot title/select a checkpoint experiment to display but I think they should be hidden with some message to say "select a checkpoint experiment to display trend plots"

A message makes sense! Not sure where it could go though 🤔. Will start experimenting with some ideas :)

Also, need to consider when there are no custom plots added but we do have data available:

What's the issue with the current setup? Text says "No Plots Added" and you can add a plot with the plus button 🤔

Copy link
Member

Choose a reason for hiding this comment

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

What's the issue with the current setup? Text says "No Plots Added" and you can add a plot with the plus button

The screen shouldn't be split. The custom section should be hidden and there should be two buttons, one to add an experiment and one to add a custom plot.

Copy link
Member

Choose a reason for hiding this comment

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

There are 3 things that can currently cause an "empty" plots screen:

  1. No plots in the project
  2. No plots selected
  3. No experiments selected

I'm about to introduce a global error too which gives 4.

I think we need to distinguish between the user having added custom plots or not.

From those we end up with the following combinations:

project has plots plots selected experiments selected project has custom plots project has global error welcome screen content buttons to add/create show or hide custom plots section
N/A N/A N/A N Y Error message custom plots hide
N N/A N/A N N Info on adding plots to the project custom plot hide
Y N N N N There are no selected plots or experiments plots, experiments, custom plots hide
Y N Y N N There are no selected plots or custom plots plots, custom plots hide
Y Y N N N The are no selected experiments or custom plots experiments, custom plots hide
N/A N/A N/A Y Y Error message custom plots show
N N/A N/A Y N Info on adding plots to the project none show
Y N N Y N There are no selected plots or experiments plots, experiments show
Y N Y Y N There are no selected plots plots show
Y Y N Y N There are no experiments selected experiments show
Y Y Y Y N Normal webview N/A show

I added suggested welcome screen content, buttons and whether I think we should show or hide the custom plots section. IMO the section should be hidden if there is no other data sent the webview and a button should be added to create new custom plots.

I even think we should do this when there is a global CLI error.

The alternative is that we throw away the welcome screen concept in favour of improving the empty state of the existing sections. We then only have to deal with the project having no data at all.

Copy link
Member

Choose a reason for hiding this comment

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

Another alternative is to simplify the welcome screen into as few states as possible.

project has plots plots selected experiments selected project has custom plots project has global error welcome screen content buttons to select/create show or hide custom plots section
N N/A N/A N N Info on adding plots to the project custom plot hide
Y N/A N/A N N There are no plots to show experiments, plots, custom plots hide
N N/A N/A Y N Info on adding plots to the project none show
Y N/A N/A Y N There are no plots to show experiments, plots, experiments show

We can deal with global errors separately but that would give us 6 states instead of 11. Seems pretty reasonable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification! Updated the logic.

)
const data = collectCustomPlots({
...defaultFuncArgs,
Expand Down
2 changes: 1 addition & 1 deletion extension/src/plots/model/collect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export const collectCustomPlots = ({
nbItemsPerRow: number
}): CustomPlotData[] => {
const plots = []
const shouldSkipCheckpointPlots = !hasCheckpoints || !selectedRevisions
const shouldSkipCheckpointPlots = !hasCheckpoints

for (const value of plotsOrderValues) {
if (shouldSkipCheckpointPlots && isCheckpointValue(value.type)) {
Expand Down
8 changes: 5 additions & 3 deletions extension/src/test/suite/experiments/model/tree.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,16 @@ suite('Experiments Tree Test Suite', () => {

expect(
messageSpy,
'when there are no experiments selected we dont send checkpoint type plots'
'when there are no experiments selected we send checkpoint plots with empty values'
).to.be.calledWithMatch({
comparison: null,
custom: {
...customPlotsFixture,
colors: undefined,
plots: customPlotsFixture.plots.filter(
plot => plot.type !== CustomPlotType.CHECKPOINT
plots: customPlotsFixture.plots.map(plot =>
plot.type === CustomPlotType.CHECKPOINT
? { ...plot, values: [] }
: plot
)
},
hasPlots: false,
Expand Down
63 changes: 22 additions & 41 deletions webview/src/plots/components/App.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,14 +334,14 @@ describe('App', () => {
})
})

it('should render an empty state given a message with only custom plots data', () => {
it('should render get started along with custom plots given a message with only custom plots data', () => {
renderAppWithOptionalData({
custom: customPlotsFixture
})

expect(screen.queryByText('Loading Plots...')).not.toBeInTheDocument()
const addExperimentsButton = screen.queryByText('Add Experiments')

expect(screen.getByText('Custom')).toBeInTheDocument()
expect(addExperimentsButton).toBeInTheDocument()
})

Expand All @@ -358,16 +358,15 @@ describe('App', () => {

it('should render custom with "No Plots Added" message when there are no plots added', () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: {
...customPlotsFixture,
plots: []
}
})

expect(screen.queryByText('Loading Plots...')).not.toBeInTheDocument()
expect(screen.queryByText('No Plots to Display')).not.toBeInTheDocument()
expect(screen.getByText('Custom')).toBeInTheDocument()
expect(screen.getByText('No Plots to Display')).toBeInTheDocument()
expect(screen.getByText('No Plots Added')).toBeInTheDocument()
})

Expand Down Expand Up @@ -404,24 +403,18 @@ describe('App', () => {
expect(emptyState).toBeInTheDocument()
})

it('should remove custom plots given a message showing custom plots as null', async () => {
const emptyStateText = 'No Plots to Display'

it('should remove custom plots given a message showing custom plots as null', () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture,
template: templatePlotsFixture
custom: customPlotsFixture
})

expect(screen.queryByText(emptyStateText)).not.toBeInTheDocument()
expect(screen.getByText('Custom')).toBeInTheDocument()

sendSetDataMessage({
custom: null
})

const emptyState = await screen.findByText(emptyStateText)

expect(emptyState).toBeInTheDocument()
expect(screen.queryByText('Custom')).not.toBeInTheDocument()
})

it('should remove all sections from the document if there is no data provided', () => {
Expand All @@ -440,7 +433,6 @@ describe('App', () => {

it('should toggle the custom plots section in state when its header is clicked', async () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})

Expand Down Expand Up @@ -475,7 +467,6 @@ describe('App', () => {

it('should not toggle the custom plots section when its header is clicked and its title is selected', async () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})

Expand Down Expand Up @@ -542,7 +533,6 @@ describe('App', () => {

it('should not toggle the custom plots section when its header is clicked and the content of its tooltip is selected', async () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})

Expand Down Expand Up @@ -576,12 +566,11 @@ describe('App', () => {

it('should display a slider to pick the number of items per row if there are items and the action is available', () => {
const store = renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})
setWrapperSize(store)

expect(screen.getAllByTestId('size-sliders')[1]).toBeInTheDocument()
expect(screen.getByTestId('size-sliders')).toBeInTheDocument()
})

it('should not display a slider to pick the number of items per row if there are no items', () => {
Expand All @@ -593,7 +582,6 @@ describe('App', () => {

it('should not display a slider to pick the number of items per row if the only width available for one item per row or less', () => {
const store = renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})
setWrapperSize(store, 400)
Expand All @@ -616,13 +604,12 @@ describe('App', () => {

it('should display both size sliders for custom plots', () => {
const store = renderAppWithOptionalData({
custom: customPlotsFixture,
template: templatePlotsFixture
custom: customPlotsFixture
})
setWrapperSize(store)

const plotResizers = within(
screen.getAllByTestId('size-sliders')[1]
screen.getByTestId('size-sliders')
).getAllByRole('slider')

expect(plotResizers.length).toBe(2)
Expand All @@ -643,14 +630,13 @@ describe('App', () => {

it('should send a message to the extension with the selected size when changing the width of plots', () => {
const store = renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})
setWrapperSize(store)

const plotResizer = within(
screen.getAllByTestId('size-sliders')[1]
).getAllByRole('slider')[0]
const plotResizer = within(screen.getByTestId('size-sliders')).getAllByRole(
'slider'
)[0]

fireEvent.change(plotResizer, { target: { value: -3 } })
expect(mockPostMessage).toHaveBeenCalledWith({
Expand All @@ -665,14 +651,13 @@ describe('App', () => {

it('should send a message to the extension with the selected size when changing the height of plots', () => {
const store = renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})
setWrapperSize(store)

const plotResizer = within(
screen.getAllByTestId('size-sliders')[1]
).getAllByRole('slider')[1]
const plotResizer = within(screen.getByTestId('size-sliders')).getAllByRole(
'slider'
)[1]

fireEvent.change(plotResizer, { target: { value: 3 } })
expect(mockPostMessage).toHaveBeenCalledWith({
Expand Down Expand Up @@ -714,7 +699,6 @@ describe('App', () => {

it('should send a message to the extension when the custom plots are reordered', () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})

Expand Down Expand Up @@ -749,7 +733,6 @@ describe('App', () => {

it('should add a custom plot if a user creates a custom plot', () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: {
...customPlotsFixture,
plots: customPlotsFixture.plots.slice(0, 3)
Expand Down Expand Up @@ -780,7 +763,6 @@ describe('App', () => {

it('should remove a custom plot if a user deletes a custom plot', () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
})

Expand Down Expand Up @@ -1294,7 +1276,6 @@ describe('App', () => {

it('should open a modal with the plot zoomed in when clicking a custom plot', () => {
renderAppWithOptionalData({
comparison: comparisonTableFixture,
custom: customPlotsFixture
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
})

Expand Down Expand Up @@ -1431,7 +1412,7 @@ describe('App', () => {
describe('Large plots', () => {
it('should wrap the custom plots in a big grid (virtualize them) when there are more than eight large plots', async () => {
await renderAppAndChangeSize(
{ comparison: comparisonTableFixture, custom: createCustomPlots(9) },
{ custom: createCustomPlots(9) },
1,
PlotsSection.CUSTOM_PLOTS
)
Expand All @@ -1449,7 +1430,7 @@ describe('App', () => {

it('should not wrap the custom plots in a big grid (virtualize them) when there are eight or fewer large plots', async () => {
await renderAppAndChangeSize(
{ comparison: comparisonTableFixture, custom: createCustomPlots(8) },
{ custom: createCustomPlots(8) },
1,
PlotsSection.CUSTOM_PLOTS
)
Expand Down Expand Up @@ -1558,7 +1539,7 @@ describe('App', () => {
describe('Regular plots', () => {
it('should wrap the custom plots in a big grid (virtualize them) when there are more than fourteen regular plots', async () => {
await renderAppAndChangeSize(
{ comparison: comparisonTableFixture, custom: createCustomPlots(15) },
{ custom: createCustomPlots(15) },
DEFAULT_NB_ITEMS_PER_ROW,
PlotsSection.CUSTOM_PLOTS
)
Expand All @@ -1568,7 +1549,7 @@ describe('App', () => {

it('should not wrap the custom plots in a big grid (virtualize them) when there are fourteen regular plots', async () => {
await renderAppAndChangeSize(
{ comparison: comparisonTableFixture, custom: createCustomPlots(14) },
{ custom: createCustomPlots(14) },
DEFAULT_NB_ITEMS_PER_ROW,
PlotsSection.CUSTOM_PLOTS
)
Expand Down Expand Up @@ -1655,7 +1636,7 @@ describe('App', () => {
describe('Smaller plots', () => {
it('should wrap the custom plots in a big grid (virtualize them) when there are more than twenty small plots', async () => {
await renderAppAndChangeSize(
{ comparison: comparisonTableFixture, custom: createCustomPlots(21) },
{ custom: createCustomPlots(21) },
4,
PlotsSection.CUSTOM_PLOTS
)
Expand All @@ -1665,7 +1646,7 @@ describe('App', () => {

it('should not wrap the custom plots in a big grid (virtualize them) when there are twenty or fewer small plots', async () => {
await renderAppAndChangeSize(
{ comparison: comparisonTableFixture, custom: createCustomPlots(20) },
{ custom: createCustomPlots(20) },
4,
PlotsSection.CUSTOM_PLOTS
)
Expand Down
2 changes: 1 addition & 1 deletion webview/src/plots/components/GetStarted.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export const AddPlots: React.FC<AddPlotsProps> = ({
hasUnselectedPlots
}: AddPlotsProps) => (
<div>
<p>No Plots to Display.</p>
<p>There are no selected plots or experiments.</p>
<div>
<StartButton
onClick={() =>
Expand Down
40 changes: 32 additions & 8 deletions webview/src/plots/components/Plots.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { TemplatePlotsWrapper } from './templatePlots/TemplatePlotsWrapper'
import { ComparisonTableWrapper } from './comparisonTable/ComparisonTableWrapper'
import { Ribbon } from './ribbon/Ribbon'
import { setMaxNbPlotsPerRow, setZoomedInPlot } from './webviewSlice'
import styles from './styles.module.scss'
import { EmptyState } from '../../shared/components/emptyState/EmptyState'
import { Modal } from '../../shared/components/modal/Modal'
import { WebviewWrapper } from '../../shared/components/webviewWrapper/WebviewWrapper'
Expand All @@ -15,15 +16,20 @@ import { PlotsState } from '../store'

const PlotsContent = () => {
const dispatch = useDispatch()
const { hasData, hasPlots, hasUnselectedPlots, zoomedInPlot } = useSelector(
(state: PlotsState) => state.webview
)
const {
hasData,
hasPlots,
hasUnselectedPlots,
zoomedInPlot,
selectedRevisions
} = useSelector((state: PlotsState) => state.webview)
const hasComparisonData = useSelector(
(state: PlotsState) => state.comparison.hasData
)
const hasTemplateData = useSelector(
(state: PlotsState) => state.template.hasData
)
const hasCustomData = useSelector((state: PlotsState) => state.custom.hasData)
const wrapperRef = createRef<HTMLDivElement>()

useLayoutEffect(() => {
Expand All @@ -49,11 +55,29 @@ const PlotsContent = () => {

if (!hasComparisonData && !hasTemplateData) {
return (
<GetStarted
addItems={<AddPlots hasUnselectedPlots={hasUnselectedPlots} />}
showEmpty={!hasPlots}
welcome={<Welcome />}
/>
<div className={styles.getStartedWrapper}>
{selectedRevisions.length > 0 && <Ribbon />}
<GetStarted
addItems={<AddPlots hasUnselectedPlots={hasUnselectedPlots} />}
showEmpty={!hasPlots}
welcome={<Welcome />}
isFullScreen={!hasCustomData}
/>
{hasCustomData && (
<>
<CustomPlotsWrapper />
{zoomedInPlot?.plot && (
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
<Modal
onClose={() => {
dispatch(setZoomedInPlot(undefined))
}}
>
<ZoomedInPlot props={zoomedInPlot.plot} />
</Modal>
)}
</>
)}
</div>
)
}

Expand Down
Loading