Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: update popover custom target story #31374

Merged
merged 4 commits into from
May 16, 2024

Conversation

spmonahan
Copy link
Contributor

@spmonahan spmonahan commented May 15, 2024

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 the setState() approach. This may be a better approach for documentation.

Related Issue(s)

This PR is motivated by user feedback.

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.

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.

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 the setState() approach.
@spmonahan spmonahan requested a review from a team as a code owner May 15, 2024 00:02
@spmonahan spmonahan requested a review from behowell May 15, 2024 00:02
@github-actions github-actions bot added this to the April Project Cycle Q1 2024 milestone May 15, 2024
Copy link

codesandbox-ci bot commented May 15, 2024

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.

@fabricteam
Copy link
Collaborator

fabricteam commented May 15, 2024

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

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

@fabricteam
Copy link
Collaborator

fabricteam commented May 15, 2024

📊 Bundle size report

✅ No changes found

@fabricteam
Copy link
Collaborator

🕵 fluentuiv9 No visual regressions between this PR and main

@spmonahan spmonahan merged commit 30e870e into microsoft:master May 16, 2024
20 checks passed
miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this pull request Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants