Add a message when we are showing only 20 plots #5008
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| import { MessageBand } from '../../shared/components/messageBand/MessageBand' | ||
| import { Info } from '../../shared/components/icons' | ||
|
|
||
| const PathHighlight: React.FC<PropsWithChildren> = ({ children }) => ( |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| <MessageBand icon={Info} id="too-many-plots-message"> | ||
| We are only showing 20 plots by default. To view other plots, you can | ||
| toggle their visibility in the sidebar. You can also group your plots by | ||
| adding a path prefix to their IDs ( |
There was a problem hiding this comment.
[C] In the dvc.yaml the ID for the plot can be a file or a string. The following are all valid (from demo project):
- Loss:
x: step
y:
training/plots/metrics/train/loss.tsv: loss
training/plots/metrics/test/loss.tsv: loss
y_label: loss
title: "Loss"
- Confusion matrix:
template: confusion
x: actual
y:
training/plots/sklearn/confusion_matrix.json: predicted
- hist.csv:
x: preds
y: digit
template: bar_horizontal
title: Histogram of Predictions
- probs.json:
x: actual
y: prob
template: scatter
title: Predicted Probabilities
- training/plots/images
This means the instructions to group need to be updated slightly. In the case of the key being a file the user would need to group the underlying data. If it is just a string then it can be done in the suggested way.
There was a problem hiding this comment.
I changed the message. Does that look like complete instructions to you now @mattseddon ?
|
Because this has already been reviewed and there were a lot of changes requested in the planning yesterday, I will leave this up and merge the changes back into this PR through another PR. Changes requested:
|
| const nodes = [{}] | ||
| for (let i = 0; i < 19; i++) { | ||
| nodes.push({}) | ||
| } |
There was a problem hiding this comment.
Minor! I think we can create nodes for this test and the next one in a single line:
const nodes = Array.from({ length: 20 }).fill({})
* Select 20 plots by default per section * Self review * Fix parent status * Simplify custom selection * Fix test
| @@ -65,6 +67,12 @@ export const comparisonTableSlice = createSlice({ | |||
| ...action.payload, | |||
| hasData: !!action.payload | |||
| } | |||
| }, | |||
| updateShouldShowTooManyPlotsMessage: ( | |||
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| @@ -93,6 +95,12 @@ export const templatePlotsSlice = createSlice({ | |||
| ...state, | |||
| sections: action.payload | |||
| } | |||
| }, | |||
| updateShouldShowTooManyPlotsMessage: ( | |||
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| const { revisions, plots, width } = useSelector( | ||
| (state: PlotsState) => state.comparison | ||
| ) | ||
| const { revisions, plots, width, shouldShowTooManyPlotsMessage } = |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
| const { nbItemsPerRow, sections, hasItems } = useSelector( | ||
| (state: PlotsState) => state.template | ||
| ) | ||
| const { nbItemsPerRow, sections, hasItems, shouldShowTooManyPlotsMessage } = |
There was a problem hiding this comment.
Similar blocks of code found in 4 locations. Consider refactoring.
|
Code Climate has analyzed commit 7389c79 and detected 10 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 99.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.0% change). View more on Code Climate. |
1/ 2 main <- this <- #5020
Part of #4867
Screen.Recording.2023-11-17.at.9.58.10.AM.mov
I wanted the message to be easily seen (always shown) but at the same time dismissible. Let me know what you think. Should we use a VS Code toast instead?