From 90b7d218212b882f123ed1065af314ff6a06ef67 Mon Sep 17 00:00:00 2001
From: Marcus Eide <eide@spotify.com>
Date: Fri, 2 Oct 2020 09:16:25 +0200
Subject: [PATCH] Refactor the updatePosition logic

---
 .../src/components/SettingsDialog.test.tsx    | 14 ++++++++----
 .../src/components/SettingsDialog.tsx         | 22 ++++++-------------
 .../src/components/UserSettings.tsx           |  8 +++++--
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/plugins/user-settings/src/components/SettingsDialog.test.tsx b/plugins/user-settings/src/components/SettingsDialog.test.tsx
index d738c2ece0..fb32e9cfdb 100644
--- a/plugins/user-settings/src/components/SettingsDialog.test.tsx
+++ b/plugins/user-settings/src/components/SettingsDialog.test.tsx
@@ -21,7 +21,8 @@ import {
   featureFlagsApiRef,
 } from '@backstage/core';
 import { renderWithEffects, wrapInTestApp } from '@backstage/test-utils';
-import * as React from 'react';
+import { fireEvent } from '@testing-library/react';
+import React from 'react';
 import { SettingsDialog } from './SettingsDialog';
 
 const apiRegistry = ApiRegistry.from([
@@ -29,13 +30,13 @@ const apiRegistry = ApiRegistry.from([
 ]);
 
 describe('<SettingsDialog />', () => {
-  const mockRef = { current: null };
+  const mockFn = jest.fn();
 
-  it('displays the users name and email, and the tabs and titles', async () => {
+  it('displays the users name and email, and the tabs and titles and updates the position', async () => {
     const rendered = await renderWithEffects(
       wrapInTestApp(
         <ApiProvider apis={apiRegistry}>
-          <SettingsDialog popoverActionRef={mockRef} />
+          <SettingsDialog updatePosition={mockFn} />
         </ApiProvider>,
       ),
     );
@@ -46,5 +47,10 @@ describe('<SettingsDialog />', () => {
     expect(rendered.getByText('Additional Settings')).toBeInTheDocument();
     expect(rendered.getByText('Auth Providers')).toBeInTheDocument();
     expect(rendered.getByText('Feature Flags')).toBeInTheDocument();
+
+    const flagButton = rendered.getByText('Feature Flags');
+    expect(mockFn).toBeCalledTimes(1);
+    fireEvent.click(flagButton);
+    expect(mockFn).toBeCalledTimes(2);
   });
 });
diff --git a/plugins/user-settings/src/components/SettingsDialog.tsx b/plugins/user-settings/src/components/SettingsDialog.tsx
index 82e744bec1..c6d9cfa8c5 100644
--- a/plugins/user-settings/src/components/SettingsDialog.tsx
+++ b/plugins/user-settings/src/components/SettingsDialog.tsx
@@ -14,14 +14,8 @@
  * limitations under the License.
  */
 
-import React, { ChangeEvent, RefObject, useEffect, useState } from 'react';
-import {
-  Card,
-  CardContent,
-  CardHeader,
-  makeStyles,
-  PopoverActions,
-} from '@material-ui/core';
+import React, { ChangeEvent, useEffect, useState } from 'react';
+import { Card, CardContent, CardHeader, makeStyles } from '@material-ui/core';
 import { AppSettingsList } from './AppSettingsList';
 import { AuthProvidersList } from './AuthProviderList';
 import { FeatureFlagsList } from './FeatureFlagsList';
@@ -47,14 +41,11 @@ const useStyles = makeStyles({
 });
 
 type Props = {
-  popoverActionRef: RefObject<PopoverActions | null>;
   providerSettings?: ProviderSettings;
+  updatePosition: () => void;
 };
 
-export const SettingsDialog = ({
-  popoverActionRef,
-  providerSettings,
-}: Props) => {
+export const SettingsDialog = ({ providerSettings, updatePosition }: Props) => {
   const classes = useStyles();
   const { profile, displayName } = useUserProfile();
   const featureFlagsApi = useApi(featureFlagsApiRef);
@@ -62,6 +53,7 @@ export const SettingsDialog = ({
   const [selectedTab, setSelectedTab] = useState<string | number>('auth');
 
   const providers = providerSettings ?? <DefaultProviderSettings />;
+  const ref = React.useRef(updatePosition);
 
   const handleChange = (
     _ev: ChangeEvent<{}>,
@@ -72,8 +64,8 @@ export const SettingsDialog = ({
 
   // Update the position of the popover to handle different heights
   useEffect(() => {
-    popoverActionRef?.current?.updatePosition();
-  }, [selectedTab, popoverActionRef]);
+    ref.current?.();
+  }, [selectedTab]);
 
   return (
     <Card className={classes.root}>
diff --git a/plugins/user-settings/src/components/UserSettings.tsx b/plugins/user-settings/src/components/UserSettings.tsx
index b117d40acb..34d7fa02be 100644
--- a/plugins/user-settings/src/components/UserSettings.tsx
+++ b/plugins/user-settings/src/components/UserSettings.tsx
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import React, { useEffect, useContext } from 'react';
+import React, { useEffect, useContext, useCallback } from 'react';
 import { Popover, PopoverActions } from '@material-ui/core';
 import { SignInAvatar } from './SignInAvatar';
 import { SettingsDialog } from './SettingsDialog';
@@ -43,6 +43,10 @@ export const UserSettings = ({ providerSettings }: Props) => {
   );
   const popoverActionRef = React.useRef<PopoverActions | null>(null);
 
+  const updatePosition = useCallback(() => {
+    popoverActionRef?.current?.updatePosition();
+  }, []);
+
   const handleOpen = (event?: React.MouseEvent<HTMLButtonElement>) => {
     setAnchorEl(event?.currentTarget ?? undefined);
     setOpen(true);
@@ -81,7 +85,7 @@ export const UserSettings = ({ providerSettings }: Props) => {
         }}
       >
         <SettingsDialog
-          popoverActionRef={popoverActionRef}
+          updatePosition={updatePosition}
           providerSettings={providerSettings}
         />
       </Popover>