Add action buttons to empty plot sections #4694
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| enablePlotCreation, | ||
| hasAddedPlots | ||
| } = useSelector((state: PlotsState) => state.custom) | ||
| const { plotsIds, nbItemsPerRow, isCollapsed, height, hasAddedPlots } = |
There was a problem hiding this comment.
Similar blocks of code found in 3 locations. Consider refactoring.
| @@ -52,24 +52,6 @@ export const getCustomPlotPathsFromColumns = ( | |||
| return { metrics, params } | |||
| } | |||
|
|
|||
| export const checkForCustomPlotOptions = ( | |||
There was a problem hiding this comment.
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.
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.
If the option fails under these conditions with a clear message then that is ok.
Yes, the option fails with an error toast message.
| @@ -100,7 +100,6 @@ export type CustomPlotData = CustomPlot & { | |||
| export type CustomPlotsData = { | |||
| plots: CustomPlotData[] | |||
| nbItemsPerRow: number | |||
| enablePlotCreation: boolean | |||
There was a problem hiding this comment.
No longer needed since our "Add Custom Plot" action buttons will only appear in the case of there being no custom plots at all.
| onClick={selectPlots} | ||
| text="Select Plots" | ||
| /> | ||
| <StartButton |
There was a problem hiding this comment.
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:
- Have the custom plot section open the "Select Plot Type" quick pick as well. Would mean an extra click for the user but would reduce some code.
- Update our "Add Data Series Plot" action to open a quick pick first instead of the file selector, allowing the user to get context and type in a relative file if preferred. Would get rid of the extra click a user needs to make, but would make data series plot creation a bit more complex.
There was a problem hiding this comment.
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 dvc.yaml being open anyway... right?
There was a problem hiding this comment.
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.
|
Demo look perfect to me. Thanks! |
|
Code Climate has analyzed commit d62de15 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.0% (0.0% change). View more on Code Climate. |
Demo
Custom
https://github.com/iterative/vscode-dvc/assets/43496356/23f4a1db-a22d-4adf-bf1e-90aa0725ab7aScreen.Recording.2023-09-21.at.8.07.01.AM.mov
Template
Screen.Recording.2023-09-20.at.9.50.30.AM.mov
Followup to #4680