From d4ee67169fe20d8a25dcff821fcc94d217c7b567 Mon Sep 17 00:00:00 2001 From: Micah Godbolt Date: Tue, 21 Feb 2023 11:14:40 -0800 Subject: [PATCH 1/4] fix possible recursive loop --- .../react/src/components/Coachmark/Coachmark.base.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/react/src/components/Coachmark/Coachmark.base.tsx b/packages/react/src/components/Coachmark/Coachmark.base.tsx index 4b4b72678011fc..3efd001be560d8 100644 --- a/packages/react/src/components/Coachmark/Coachmark.base.tsx +++ b/packages/react/src/components/Coachmark/Coachmark.base.tsx @@ -431,6 +431,7 @@ export const CoachmarkBase: React.FunctionComponent = React.for target, color, positioningContainerProps, + isPositionForced, ariaDescribedBy, ariaDescribedByText, ariaLabelledBy, @@ -471,9 +472,9 @@ export const CoachmarkBase: React.FunctionComponent = React.for const async = useAsync(); const [bounds, setBounds] = React.useState(); const updateAsyncPosition = (): void => { - async.requestAnimationFrame(() => setBounds(getBounds(props))); + async.requestAnimationFrame(() => setBounds(getBounds({ isPositionForced, positioningContainerProps }))); }; - React.useEffect(updateAsyncPosition); + React.useEffect(updateAsyncPosition, [async]); return bounds; } @@ -536,7 +537,10 @@ export const CoachmarkBase: React.FunctionComponent = React.for }); CoachmarkBase.displayName = COMPONENT_NAME; -function getBounds({ isPositionForced, positioningContainerProps }: ICoachmarkProps): IRectangle | undefined { +function getBounds({ + isPositionForced, + positioningContainerProps, +}: Pick): IRectangle | undefined { if (isPositionForced) { // If directionalHint direction is the top or bottom auto edge, then we want to set the left/right bounds // to the window x-axis to have auto positioning work correctly. From a06a8e95360e414e419068fe93afc069f4cd9710 Mon Sep 17 00:00:00 2001 From: Micah Godbolt Date: Tue, 21 Feb 2023 11:16:22 -0800 Subject: [PATCH 2/4] change --- ...luentui-react-f312783a-5543-4ccb-bb23-86fa0c830e89.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-f312783a-5543-4ccb-bb23-86fa0c830e89.json diff --git a/change/@fluentui-react-f312783a-5543-4ccb-bb23-86fa0c830e89.json b/change/@fluentui-react-f312783a-5543-4ccb-bb23-86fa0c830e89.json new file mode 100644 index 00000000000000..e585bb567d5b32 --- /dev/null +++ b/change/@fluentui-react-f312783a-5543-4ccb-bb23-86fa0c830e89.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: Coachmark remove possible recursive loop", + "packageName": "@fluentui/react", + "email": "mgodbolt@microsoft.com", + "dependentChangeType": "patch" +} From 9d2c38d5e311c5256e465b426334be1eed8262f9 Mon Sep 17 00:00:00 2001 From: Micah Godbolt Date: Tue, 21 Feb 2023 11:20:56 -0800 Subject: [PATCH 3/4] add in additional deps that linting didnt require --- packages/react/src/components/Coachmark/Coachmark.base.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react/src/components/Coachmark/Coachmark.base.tsx b/packages/react/src/components/Coachmark/Coachmark.base.tsx index 3efd001be560d8..2da4bbd14e69ca 100644 --- a/packages/react/src/components/Coachmark/Coachmark.base.tsx +++ b/packages/react/src/components/Coachmark/Coachmark.base.tsx @@ -474,7 +474,8 @@ export const CoachmarkBase: React.FunctionComponent = React.for const updateAsyncPosition = (): void => { async.requestAnimationFrame(() => setBounds(getBounds({ isPositionForced, positioningContainerProps }))); }; - React.useEffect(updateAsyncPosition, [async]); + // eslint-disable-next-line react-hooks/exhaustive-deps + React.useEffect(updateAsyncPosition, [async, isPositionForced, positioningContainerProps]); return bounds; } From 58f3ce8e03bc02e1c511993fc7fb35d4fc6f4b43 Mon Sep 17 00:00:00 2001 From: Sean Monahan Date: Tue, 21 Feb 2023 20:34:44 +0000 Subject: [PATCH 4/4] fix: refactor useGetBounds Moves useGetBounds() to module scope so the hook is not recreated on every render. This also ensures we're not relying on component scope to consume props. Updates useGetBounds() to use useEffect()'s dependency array to prevent continual re-renders. --- .../components/Coachmark/Coachmark.base.tsx | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/packages/react/src/components/Coachmark/Coachmark.base.tsx b/packages/react/src/components/Coachmark/Coachmark.base.tsx index 2da4bbd14e69ca..6ffe5df171362a 100644 --- a/packages/react/src/components/Coachmark/Coachmark.base.tsx +++ b/packages/react/src/components/Coachmark/Coachmark.base.tsx @@ -23,6 +23,7 @@ import { FocusTrapZone } from '../../FocusTrapZone'; import { useAsync, useOnEvent, useSetTimeout, useWarnings } from '@fluentui/react-hooks'; import type { IRectangle } from '../../Utilities'; import type { IPositionedData } from '../../Positioning'; +import type { IPositioningContainerProps } from './PositioningContainer/PositioningContainer.types'; import type { ICoachmarkProps, ICoachmarkStyles, ICoachmarkStyleProps } from './Coachmark.types'; import type { IBeakProps } from './Beak/Beak.types'; @@ -401,6 +402,15 @@ function useDeprecationWarning(props: ICoachmarkProps) { } } +function useGetBounds(props: ICoachmarkProps): IRectangle | undefined { + const async = useAsync(); + const [bounds, setBounds] = React.useState(); + React.useEffect(() => { + async.requestAnimationFrame(() => setBounds(getBounds(props.isPositionForced, props.positioningContainerProps))); + }, [async, props.isPositionForced, props.positioningContainerProps]); + return bounds; +} + const COMPONENT_NAME = 'CoachmarkBase'; export const CoachmarkBase: React.FunctionComponent = React.forwardRef< @@ -431,7 +441,6 @@ export const CoachmarkBase: React.FunctionComponent = React.for target, color, positioningContainerProps, - isPositionForced, ariaDescribedBy, ariaDescribedByText, ariaLabelledBy, @@ -468,17 +477,6 @@ export const CoachmarkBase: React.FunctionComponent = React.for const finalHeight: number | undefined = isCollapsed ? COACHMARK_HEIGHT : entityInnerHostRect.height; - function useGetBounds(): IRectangle | undefined { - const async = useAsync(); - const [bounds, setBounds] = React.useState(); - const updateAsyncPosition = (): void => { - async.requestAnimationFrame(() => setBounds(getBounds({ isPositionForced, positioningContainerProps }))); - }; - // eslint-disable-next-line react-hooks/exhaustive-deps - React.useEffect(updateAsyncPosition, [async, isPositionForced, positioningContainerProps]); - return bounds; - } - return ( = React.for finalHeight={finalHeight} ref={forwardedRef} onPositioned={onPositioned} - bounds={useGetBounds()} + bounds={useGetBounds(props)} {...positioningContainerProps} >
@@ -538,10 +536,10 @@ export const CoachmarkBase: React.FunctionComponent = React.for }); CoachmarkBase.displayName = COMPONENT_NAME; -function getBounds({ - isPositionForced, - positioningContainerProps, -}: Pick): IRectangle | undefined { +function getBounds( + isPositionForced?: boolean, + positioningContainerProps?: IPositioningContainerProps, +): IRectangle | undefined { if (isPositionForced) { // If directionalHint direction is the top or bottom auto edge, then we want to set the left/right bounds // to the window x-axis to have auto positioning work correctly.