fix: fix flickering when expanding/collapsing sections #595
Conversation
| </div> | ||
| )} | ||
| </Disclosure.Summary> | ||
| {type === 'image' ? this._renderContent() : |
There was a problem hiding this comment.
why don't you need this condition now?
There was a problem hiding this comment.
We wrap components in Card directly where we need it and pass already wrapped components as children.
The Details component shouldn't be aware of anything regarding its children IMO. It shouldn't care what it will render, it just receives ReactNode and renders it as-is. That makes logic a lot easier to understand and reduces complexity. That's from the software design point of view.
Another reason why I got rid of it, because it causes another visual glitch — upon collapsing Meta/History, you'd first see a frame where Disclosure contains empty card and only afterwards it collapses completely.
| @@ -0,0 +1,3 @@ | |||
| import {createContext} from 'react'; | |||
|
|
|||
| export const MeasurementContext = createContext({measure: () => {}}); | |||
There was a problem hiding this comment.
can't figure out how it works and fixes the problem
There was a problem hiding this comment.
Intro: react-virtualized needs to know exact sizes of each element for correct positioning. To let react-virtualized know the exact dimensions of element, you should use measure() function that is provided by <CellMeasurer/>.
Problem (how it was before): when we expand section, it immediately renders expanded section. At this point react-virtualized knows nothing about expanded section, because measure() function wasn't called. On the next frame ResizeObserver detects a change and calls measure() that adjusts list items positioning. User sees intermediate frame and perceives it as glitch.
Solution (how it is now): we use useLayoutEffect hook inside elements that change size instead of relying solely on ResizeObserver. Now, when we expand section, the component re-renders, but before painting on the screen useLayoutEffect is called, where we call measure() to let react-virtualized adjust positioning before user sees anything.
Since there are many components that need to be able to call measure() before rendering (section, browser section, screenshot state, meta info, etc.) I introduced MeasurementContext just to avoid prop drilling. Architecture-wise it may be far from perfect (ideally components shouldn't be aware of this), but I think it's good enough before we switch to new UI.
Now, everything looks 100% correct and user never sees intermediate state. However there's definitely some room for optimisation in terms of performance. If you click on expand/collapse all under the devtools CPU x6 slower throttling, it takes around 5s to render. That's because measuring happens tens of times instead of just once before final render. I wasn't able to fix this quickly, and without throttling everything works fine, so decided to keep it as is.
shadowusr
force-pushed
the
users/shadowusr/INFRADUTY-27472
branch
from
82b6007 to
f0b22fd
Compare
| // eslint-disable-next-line no-use-before-define | ||
| return <SectionCommonConnected |
There was a problem hiding this comment.
Maybe we should use regular function for defining SectionCommonConnected instead of the arrow one then?
Test report: https://nda.ya.ru/t/OhIDE9f-77hGkc