-
Notifications
You must be signed in to change notification settings - Fork 29
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 action buttons to empty plot sections #4694
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,6 @@ export type CustomPlotData = CustomPlot & { | |
export type CustomPlotsData = { | ||
plots: CustomPlotData[] | ||
nbItemsPerRow: number | ||
enablePlotCreation: boolean | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer needed since our "Add Custom Plot" action buttons will only appear in the case of there being no custom plots at all. |
||
height: PlotHeight | ||
hasUnfilteredExperiments: boolean | ||
hasAddedPlots: boolean | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,17 +5,11 @@ import { CustomPlots } from './CustomPlots' | |
import { changeSize } from './customPlotsSlice' | ||
import { PlotsContainer } from '../PlotsContainer' | ||
import { PlotsState } from '../../store' | ||
import { addCustomPlot, removeCustomPlots } from '../../util/messages' | ||
import { removeCustomPlots } from '../../util/messages' | ||
|
||
export const CustomPlotsWrapper: React.FC = () => { | ||
const { | ||
plotsIds, | ||
nbItemsPerRow, | ||
isCollapsed, | ||
height, | ||
enablePlotCreation, | ||
hasAddedPlots | ||
} = useSelector((state: PlotsState) => state.custom) | ||
const { plotsIds, nbItemsPerRow, isCollapsed, height, hasAddedPlots } = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar blocks of code found in 3 locations. Consider refactoring. |
||
useSelector((state: PlotsState) => state.custom) | ||
const [selectedPlots, setSelectedPlots] = useState<string[]>([]) | ||
useEffect(() => { | ||
setSelectedPlots(plotsIds) | ||
|
@@ -29,9 +23,6 @@ export const CustomPlotsWrapper: React.FC = () => { | |
sectionKey={PlotsSection.CUSTOM_PLOTS} | ||
nbItemsPerRowOrWidth={nbItemsPerRow} | ||
sectionCollapsed={isCollapsed} | ||
addPlotsButton={ | ||
enablePlotCreation ? { onClick: addCustomPlot } : undefined | ||
} | ||
removePlotsButton={ | ||
hasAddedPlots ? { onClick: removeCustomPlots } : undefined | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import React from 'react' | ||
import { StartButton } from '../../../shared/components/button/StartButton' | ||
import { EmptyState } from '../../../shared/components/emptyState/EmptyState' | ||
import { addCustomPlot } from '../../util/messages' | ||
|
||
export const NoPlotsAdded: React.FC = () => { | ||
return ( | ||
<EmptyState isFullScreen={false}> | ||
<p>No Plots Added</p> | ||
<StartButton onClick={addCustomPlot} text="Add Plot" /> | ||
</EmptyState> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import React from 'react' | ||
import { useSelector } from 'react-redux' | ||
import { StartButton } from '../../../shared/components/button/StartButton' | ||
import { EmptyState } from '../../../shared/components/emptyState/EmptyState' | ||
import { addPlot, selectPlots } from '../../util/messages' | ||
import { PlotsState } from '../../store' | ||
|
||
export const NoPlotsToDisplay: React.FC = () => { | ||
const { hasUnselectedPlots } = useSelector( | ||
(state: PlotsState) => state.webview | ||
) | ||
return ( | ||
<EmptyState isFullScreen={false}> | ||
<p>No Plots to Display</p> | ||
{hasUnselectedPlots ? ( | ||
<div> | ||
<StartButton | ||
isNested={true} | ||
onClick={selectPlots} | ||
text="Select Plots" | ||
/> | ||
<StartButton | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The custom section "Add Plot" button is a bit different than the template one since it can take you to right to custom plot creation, unlike the data series which needs that extra description for more context. We could keep as is, but if we wanted them to function the same we could:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I vote for option 1. The only drawback that I see is that the plot may not necessarily appear in the section that the user is currently looking at. However, I think that is fine. "Add Data Series Plot" ends with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, "Add Data Series" ends with the dvc.yaml being opened. Makes sense! I'll update the code to use the add plot command instead. |
||
text="Add Plot" | ||
appearance="secondary" | ||
isNested={true} | ||
onClick={addPlot} | ||
/> | ||
</div> | ||
) : ( | ||
<StartButton text="Add Plot" onClick={addPlot} /> | ||
)} | ||
</EmptyState> | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently our "Add Plot" shows the custom plot option even if you can't create any more custom plots. We could technically add an extra check but then our choose plot quick pick would have only one item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the option fails under these conditions with a clear message then that is ok.
I think the chances of real users hitting this is low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the option fails with an error toast message.