Add plots error message below ribbon #5165
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| @@ -43,15 +44,7 @@ export const ZoomedInPlot: React.FC<ZoomedInPlotProps> = ({ | |||
| openActionsMenu | |||
| }: ZoomedInPlotProps) => { | |||
| const zoomedInPlotRef = useRef<HTMLDivElement>(null) | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
Moved this useEffect into a hook so we could use it in both ZoomedInPlot and ErrorsModal
| @@ -95,6 +96,7 @@ export const Ribbon: React.FC = () => { | |||
| onClear={() => removeRevision(revision.id)} | |||
| /> | |||
| ))} | |||
| <Errors /> | |||
There was a problem hiding this comment.
As always, any comments/ideas for improving the design are welcome!
| @@ -83,6 +86,29 @@ const Template: StoryFn<{ | |||
| export const WithData = Template.bind({}) | |||
| WithData.parameters = CHROMATIC_VIEWPORTS_WITH_DELAY | |||
|
|
|||
| export const WithPlotsErrors = Template.bind({}) | |||
There was a problem hiding this comment.
Added a storybook for both revision and plot errors.
|
@julieg18 what happens when you scroll it? can you show on a demo please? also it seems it jumps a bit when you show the popup / close it. |
It's currently fixed to the ribbon: Screen.Recording.2024-01-05.at.9.41.20.AM.movThough looking at it when we're scrolling, I think we can decrease its height a bit. It's currently feels taller than the ribbon 🤔
Yes, I believe it's due to the fact that we are removing the scroll bar when a popup opens. Our plot modals work the same: Screen.Recording.2024-01-05.at.9.43.49.AM.mov |
|
Thanks @julieg18 , I think it looks good. One suggestion - get rid of the Errors Found and just move "Show 2 errors" there instead. I don't think we need both in this case. |
| @@ -81,6 +82,9 @@ export const feedStore = ( | |||
| case PlotsDataKeys.HAS_UNSELECTED_PLOTS: | |||
| dispatch(updateHasUnselectedPlots(!!data.data[key])) | |||
| continue | |||
| case PlotsDataKeys.PLOT_ERRORS: | |||
There was a problem hiding this comment.
This is a note to self, we should reduce the number of branches in this switch/case by having an enum instead:
const actionToDispatch = {
...
[PlotsDataKeys.PLOT_ERRORS]: updatePlotErrors
[PlotsDataKeys.SELECTED_REVISIONS]: updateSelectedRevisions
...
}
and a simple dispatch(actionToDispatch[key](data.data.key))
There was a problem hiding this comment.
I'll open an issue for that. And don't worry, it's totally out of scope and has nothing to do with your PR.
| } | ||
|
|
||
| return ( | ||
| <li className={styles.errors}> |
There was a problem hiding this comment.
It does not make much sense for this to be an <li/> element. I know that it is inside of the ribbon, but it does not fit with the rest of the list. The ribbon should be a wrapper that wraps the list and the errors.
There was a problem hiding this comment.
Makes sense! I'll take care of this in a followup since the CSS will need to be adjusted and I don't want to accidentally break the ribbon.
| </th> | ||
| </tr> | ||
| {revs.map(({ rev, msg }) => ( | ||
| <tr key={`${rev}-${msg}`}> |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
|
Code Climate has analyzed commit 780f737 and detected 5 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 98.4% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.3% (0.0% change). View more on Code Climate. |
Demo
https://github.com/iterative/vscode-dvc/assets/43496356/8f55176b-8f49-436e-a255-4eece8a9d729Screen.Recording.2024-01-05.at.4.39.03.PM.mov
Part of #5087