Fix timing of comparison table renders #1759
Conversation
|
Code Climate has analyzed commit 9dd614b and detected 0 issues on this pull request. 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.8% (0.0% change). View more on Code Climate. |
| @@ -26,6 +27,7 @@ export const ComparisonTable: React.FC<ComparisonTableProps> = ({ | |||
| }) => { | |||
| const pinnedColumn = useRef(currentPinnedColumn || '') | |||
| const [columns, setColumns] = useState<ComparisonTableColumn[]>([]) | |||
| const [comparisonPlots, setComparisonPlots] = useState<ComparisonPlots>([]) | |||
There was a problem hiding this comment.
[F] Because columns goes through useState and useEffect there was a timing issue between the plots data being updated and the columns being updated.
The plots data would be updated first and that would leave the column with no plot, hence the missing plot would be shown. In our project there is only one image but when there are more it would look even worse.
Please LMK if there is a more "correct React usage" way to fix this.
Thanks
There was a problem hiding this comment.
The problem here is that we do not want to re-render when the plots prop change. Your solution works, but in the end, it simply delays the re-render (thecomparisonPlots re-render might happen before the columns one too). So now we end up with 2 renders when the plots prop change while we simply did not want to re-render at all (once when the prop change and once when we trigger a state change after).
I do not have a solution and I do realize that I was the one who set up the thing in the first place. I think we'd need to map the columns and the plots outside of this component somehow and simply re-order in here.
We can merge this as a band-aid for now and I can either take a look after or we can pair and look at this together.
There was a problem hiding this comment.
We can pair. I'll set it up.
This PR fixes an issue with the way that the comparison table rendered. We were incorrectly rendering the "missing plot" whenever a plot was added/removed from the table. More details inline.
Before
Screen.Recording.2022-05-24.at.3.36.49.pm.mov
(I only had to see this a few times before I had to fix it 🤯 )
After
Screen.Recording.2022-05-24.at.3.35.54.pm.mov