Closed spmonahan closed 2 weeks ago
This pull request is automatically built and testable in CodeSandbox.
To see build info of the built libraries, click here or the icon next to each commit SHA.
@fluentui/react-components
)No significant results to display.
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 617 | 646 | 5000 | |
Button | mount | 310 | 314 | 5000 | |
Field | mount | 1119 | 1097 | 5000 | |
FluentProvider | mount | 715 | 714 | 5000 | |
FluentProviderWithTheme | mount | 78 | 82 | 10 | |
FluentProviderWithTheme | virtual-rerender | 36 | 33 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 76 | 71 | 10 | |
MakeStyles | mount | 864 | 862 | 50000 | |
Persona | mount | 1730 | 1659 | 5000 | |
SpinButton | mount | 1384 | 1370 | 5000 | |
SwatchPicker | mount | 1510 | 1527 | 5000 |
â No changes found
Previous Behavior
Previously the example relied on React refs as dependencies to a
useEffect()
call but these will never trigger the effect. The example still works but when trying to extrapolate from it and target elements that appear after the first render, the example breaks down because the effect will not be triggered.New Behavior
This example takes advantage of the fact that useMergedRefs() supports functions as refs so we can trigger the effect when the refs are set.
Using
setState()
was also considered but this will always trigger a re-render, negatively affecting performance so this pattern, while perhaps useful in some cases, is not a good example for best practices documentation.setState()
is the approach demonstrated in the positioning docs (link)The
useEffect()
could have its dependencies removed so it would be called every render which should allow it to function as needed with less of a performance overhead than thesetState()
approach. This may be a better approach for documentation.Related Issue(s)
This PR is motivated by user feedback.