Plot list virtualization #1707
Plot list virtualization #1707
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| @@ -85,6 +106,14 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({ | |||
| element: <DropTarget />, | |||
| wrapperTag: 'div' | |||
| }} | |||
| wrapperComponent={ | |||
There was a problem hiding this comment.
Identical blocks of code found in 2 locations. Consider refactoring.
There was a problem hiding this comment.
I'll try to remove having to pass the component to the DragDropContainer altogether.
There was a problem hiding this comment.
[Q] Do we think there will be a different component that will wrap the d'n'd container? If not then why not be specific and give it a needsBigGrid or shouldVirtualize prop?
There was a problem hiding this comment.
The comparison table should have drag and drop rows (not implemented yet), if we do virtualize them, it won't be a grid, but a list. We could also pass a VirtualizedWith and then use an enum for values. I still think the DragDropContainer should not have to deal with that component, I think I can do something with the children instead.
| @@ -99,6 +105,14 @@ export const TemplatePlotsGrid: React.FC<TemplatePlotsGridProps> = ({ | |||
| element: <DropTarget />, | |||
| wrapperTag: 'div' | |||
| }} | |||
| wrapperComponent={ | |||
There was a problem hiding this comment.
Identical blocks of code found in 2 locations. Consider refactoring.
Screen.Recording.2022-05-17.at.2.29.44.PM.mov |
| expect(screen.queryByRole('grid')).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| describe('Sizing', () => { |
There was a problem hiding this comment.
All of these tests look similar and seem like they aren't testing what their name implies, but depending on plot size and screen size, the number of items per row changes and react-virtualized grid renders the items according to their column and row index, which is calculated with the nbItemsPerRow prop. If it does not use the right number, the plots will be rendered in another order and there might be plots missing.
| changePlotsSizes: ((size: PlotSize, section: Section) => void) | undefined | ||
| } | ||
|
|
||
| export const PlotsSizeContext = createContext<PlotsSizeContextValue>({ |
There was a problem hiding this comment.
This acts as a global state for the sizes, but will re-render everything when we change the size of a plot section, while using a store would only re-render what uses it.
There was a problem hiding this comment.
This is really good. I had some minor comments throughout the PR but they were only suggestions so feel free to disregard them.
I did notice some weirdness with dragging and dropping within storybook:
Screen.Recording.2022-05-18.at.8.20.41.pm.mov
Virtualized plots being dragged into view maybe? Hard to tell when all the plots are the same.
The placeholder for a new template section is also a bit oversized now:
Screen.Recording.2022-05-18.at.8.34.41.pm.mov
Happy to merge and follow up to fix the bugs.
| </DragDropProvider> | ||
|
|
||
| {zoomedInPlot && ( |
There was a problem hiding this comment.
[Q] Should this have always been outside of the d'n'd provider?
There was a problem hiding this comment.
It does not really matter as we can't really drag and drop while zoomed in, but still since those two providers do nothing on zoomed in plot and that it re-renders every children on change, I thought it best to remove it from there.
| changePlotsSizes: ((size: PlotSize, section: Section) => void) | undefined | ||
| } | ||
|
|
||
| export const PlotsSizeContext = createContext<PlotsSizeContextValue>({ |
| @@ -85,6 +106,14 @@ export const CheckpointPlots: React.FC<CheckpointPlotsProps> = ({ | |||
| element: <DropTarget />, | |||
| wrapperTag: 'div' | |||
| }} | |||
| wrapperComponent={ | |||
There was a problem hiding this comment.
[Q] Do we think there will be a different component that will wrap the d'n'd container? If not then why not be specific and give it a needsBigGrid or shouldVirtualize prop?
|
Drag and drop has been fixed: Screen.Recording.2022-05-18.at.4.16.27.PM.movScreen.Recording.2022-05-18.at.4.16.08.PM.mov |
|
Code Climate has analyzed commit 7ce71ae and detected 2 issues 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 96.7% (0.1% change). View more on Code Climate. |
Scroll down for video (I tried many different things to get it to upload in the description, but always hit an error).