From 9684dd51456a9457a0548df3763190170e871cd6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 3 May 2024 16:01:01 +0100 Subject: [PATCH] Make TabbedView a controlled component (#12480) * Convert tabbedview to functional component The 'Tab' is still a class, so now it's a functional component that has a supporting class, which is maybe a bit... jarring, but I think is actually perfectly logical. * put comment back * Fix bad tab ID behaviour * Make TabbedView a controlled component This does mean the logic of keeping what tab is active is now in each container component, but for a functional component, this is a single line. It makes TabbedView simpler and the container components always know exactly what tab is being displayed rather than having to effectively keep the state separately themselves if they wanted it. Based on https://github.com/matrix-org/matrix-react-sdk/pull/12478 * Fix some types & unused prop * Remove weird behaviour of using first tab is active isn't valid * Don't pass initialTabID here now it no longer has the prop * Fix test * bleh... id, not icon * Change to sub-components and use contitional call syntax * Comments * Fix element IDs * Fix merge * Test DesktopCapturerSourcePicker to make sonarcloud the right colour * Use custom hook for the fllback tab behaviour --- src/components/structures/TabbedView.tsx | 47 ++++---- src/components/views/dialogs/InviteDialog.tsx | 2 +- .../views/dialogs/RoomSettingsDialog.tsx | 12 ++- .../views/dialogs/SpacePreferencesDialog.tsx | 5 +- .../views/dialogs/SpaceSettingsDialog.tsx | 4 +- .../views/dialogs/UserSettingsDialog.tsx | 11 +- .../elements/DesktopCapturerSourcePicker.tsx | 21 ++-- src/utils/DialogOpener.ts | 1 - .../components/structures/TabbedView-test.tsx | 62 +++-------- .../DesktopCapturerSourcePicker-test.tsx | 100 ++++++++++++++++++ 10 files changed, 170 insertions(+), 95 deletions(-) create mode 100644 test/components/views/elements/DesktopCapturerSourcePicker-test.tsx diff --git a/src/components/structures/TabbedView.tsx b/src/components/structures/TabbedView.tsx index d6b88b1d4f..fa717126e5 100644 --- a/src/components/structures/TabbedView.tsx +++ b/src/components/structures/TabbedView.tsx @@ -18,7 +18,6 @@ limitations under the License. import * as React from "react"; import classNames from "classnames"; -import { logger } from "matrix-js-sdk/src/logger"; import { _t, TranslationKey } from "../../languageHandler"; import AutoHideScrollbar from "./AutoHideScrollbar"; @@ -47,6 +46,18 @@ export class Tab { ) {} } +export function useActiveTabWithDefault( + tabs: NonEmptyArray>, + defaultTabID: T, + initialTabID?: T, +): [T, (tabId: T) => void] { + const [activeTabId, setActiveTabId] = React.useState( + initialTabID && tabs.some((t) => t.id === initialTabID) ? initialTabID : defaultTabID, + ); + + return [activeTabId, setActiveTabId]; +} + export enum TabLocation { LEFT = "left", TOP = "top", @@ -113,12 +124,12 @@ function TabLabel({ tab, isActive, onClick }: ITabLabelProps { // An array of objects representign tabs that the tabbed view will display. tabs: NonEmptyArray>; - // The ID of the tab to display initially. - initialTabId?: T; + // The ID of the tab to show + activeTabId: T; // The location of the tabs, dictating the layout of the TabbedView. tabLocation?: TabLocation; - // A callback that is called when the active tab changes. - onChange?: (tabId: T) => void; + // A callback that is called when the active tab should change + onChange: (tabId: T) => void; // The screen name to report to Posthog. screenName?: ScreenName; } @@ -130,39 +141,19 @@ interface IProps { export default function TabbedView(props: IProps): JSX.Element { const tabLocation = props.tabLocation ?? TabLocation.LEFT; - const [activeTabId, setActiveTabId] = React.useState((): T => { - const initialTabIdIsValid = props.tabs.find((tab) => tab.id === props.initialTabId); - // unfortunately typescript doesn't infer the types coorectly if the null check is included above - return initialTabIdIsValid && props.initialTabId ? props.initialTabId : props.tabs[0].id; - }); - const getTabById = (id: T): Tab | undefined => { return props.tabs.find((tab) => tab.id === id); }; - /** - * Shows the given tab - * @param {Tab} tab the tab to show - */ - const setActiveTab = (tab: Tab): void => { - // make sure this tab is still in available tabs - if (!!getTabById(tab.id)) { - props.onChange?.(tab.id); - setActiveTabId(tab.id); - } else { - logger.error("Could not find tab " + tab.label + " in tabs"); - } - }; - const labels = props.tabs.map((tab) => ( setActiveTab(tab)} + isActive={tab.id === props.activeTabId} + onClick={() => props.onChange(tab.id)} /> )); - const tab = getTabById(activeTabId); + const tab = getTabById(props.activeTabId); const panel = tab ? : null; const tabbedViewClasses = classNames({ diff --git a/src/components/views/dialogs/InviteDialog.tsx b/src/components/views/dialogs/InviteDialog.tsx index 2b3c7af8db..58d2d28a11 100644 --- a/src/components/views/dialogs/InviteDialog.tsx +++ b/src/components/views/dialogs/InviteDialog.tsx @@ -1538,7 +1538,7 @@ export default class InviteDialog extends React.PureComponent diff --git a/src/components/views/dialogs/RoomSettingsDialog.tsx b/src/components/views/dialogs/RoomSettingsDialog.tsx index a58cef95a7..213ee94aca 100644 --- a/src/components/views/dialogs/RoomSettingsDialog.tsx +++ b/src/components/views/dialogs/RoomSettingsDialog.tsx @@ -56,11 +56,12 @@ export const enum RoomSettingsTab { interface IProps { roomId: string; onFinished: (success?: boolean) => void; - initialTabId?: string; + initialTabId?: RoomSettingsTab; } interface IState { room: Room; + activeTabId: RoomSettingsTab; } class RoomSettingsDialog extends React.Component { @@ -70,7 +71,7 @@ class RoomSettingsDialog extends React.Component { super(props); const room = this.getRoom(); - this.state = { room }; + this.state = { room, activeTabId: props.initialTabId || RoomSettingsTab.General }; } public componentDidMount(): void { @@ -128,6 +129,10 @@ class RoomSettingsDialog extends React.Component { if (event.getType() === EventType.RoomJoinRules) this.forceUpdate(); }; + private onTabChange = (tabId: RoomSettingsTab): void => { + this.setState({ activeTabId: tabId }); + }; + private getTabs(): NonEmptyArray> { const tabs: Tab[] = []; @@ -246,8 +251,9 @@ class RoomSettingsDialog extends React.Component {
diff --git a/src/components/views/dialogs/SpacePreferencesDialog.tsx b/src/components/views/dialogs/SpacePreferencesDialog.tsx index 8e8e4ce398..2ba3a7bec6 100644 --- a/src/components/views/dialogs/SpacePreferencesDialog.tsx +++ b/src/components/views/dialogs/SpacePreferencesDialog.tsx @@ -33,7 +33,6 @@ import SettingsSubsection, { SettingsSubsectionText } from "../settings/shared/S interface IProps { space: Room; - initialTabId?: SpacePreferenceTab; onFinished(): void; } @@ -68,7 +67,7 @@ const SpacePreferencesAppearanceTab: React.FC> = ({ space ); }; -const SpacePreferencesDialog: React.FC = ({ space, initialTabId, onFinished }) => { +const SpacePreferencesDialog: React.FC = ({ space, onFinished }) => { const tabs: NonEmptyArray> = [ new Tab( SpacePreferenceTab.Appearance, @@ -90,7 +89,7 @@ const SpacePreferencesDialog: React.FC = ({ space, initialTabId, onFinis
- + {}} />
); diff --git a/src/components/views/dialogs/SpaceSettingsDialog.tsx b/src/components/views/dialogs/SpaceSettingsDialog.tsx index 0318e1af62..016307d899 100644 --- a/src/components/views/dialogs/SpaceSettingsDialog.tsx +++ b/src/components/views/dialogs/SpaceSettingsDialog.tsx @@ -82,6 +82,8 @@ const SpaceSettingsDialog: React.FC = ({ matrixClient: cli, space, onFin ].filter(Boolean) as NonEmptyArray>; }, [cli, space, onFinished]); + const [activeTabId, setActiveTabId] = React.useState(SpaceSettingsTab.General); + return ( = ({ matrixClient: cli, space, onFin fixedWidth={false} >
- +
); diff --git a/src/components/views/dialogs/UserSettingsDialog.tsx b/src/components/views/dialogs/UserSettingsDialog.tsx index a6c82d50c2..2b28e423ed 100644 --- a/src/components/views/dialogs/UserSettingsDialog.tsx +++ b/src/components/views/dialogs/UserSettingsDialog.tsx @@ -17,7 +17,7 @@ limitations under the License. import React from "react"; -import TabbedView, { Tab } from "../../structures/TabbedView"; +import TabbedView, { Tab, useActiveTabWithDefault } from "../../structures/TabbedView"; import { _t, _td } from "../../../languageHandler"; import GeneralUserSettingsTab from "../settings/tabs/user/GeneralUserSettingsTab"; import SettingsStore from "../../../settings/SettingsStore"; @@ -173,6 +173,8 @@ export default function UserSettingsDialog(props: IProps): JSX.Element { return tabs as NonEmptyArray>; }; + const [activeTabId, setActiveTabId] = useActiveTabWithDefault(getTabs(), UserTab.General, props.initialTabId); + return ( // XXX: SDKContext is provided within the LoggedInView subtree. // Modals function outside the MatrixChat React tree, so sdkContext is reprovided here to simulate that. @@ -185,7 +187,12 @@ export default function UserSettingsDialog(props: IProps): JSX.Element { title={_t("common|settings")} >
- +
diff --git a/src/components/views/elements/DesktopCapturerSourcePicker.tsx b/src/components/views/elements/DesktopCapturerSourcePicker.tsx index e4d52a8104..18cb9e6f4e 100644 --- a/src/components/views/elements/DesktopCapturerSourcePicker.tsx +++ b/src/components/views/elements/DesktopCapturerSourcePicker.tsx @@ -85,8 +85,6 @@ export interface PickerIProps { onFinished(source?: DesktopCapturerSource): void; } -type TabId = "screen" | "window"; - export default class DesktopCapturerSourcePicker extends React.Component { public interval?: number; @@ -127,15 +125,15 @@ export default class DesktopCapturerSourcePicker extends React.Component { - this.setState({ selectedSource: undefined }); + private onTabChange = (tab: Tabs): void => { + this.setState({ selectedSource: undefined, selectedTab: tab }); }; private onCloseClick = (): void => { this.props.onFinished(); }; - private getTab(type: TabId, label: TranslationKey): Tab { + private getTab(type: Tabs, label: TranslationKey): Tab { const sources = this.state.sources .filter((source) => source.id.startsWith(type)) .map((source) => { @@ -153,9 +151,9 @@ export default class DesktopCapturerSourcePicker extends React.Component> = [ - this.getTab("screen", _td("voip|screenshare_monitor")), - this.getTab("window", _td("voip|screenshare_window")), + const tabs: NonEmptyArray> = [ + this.getTab(Tabs.Screens, _td("voip|screenshare_monitor")), + this.getTab(Tabs.Windows, _td("voip|screenshare_window")), ]; return ( @@ -164,7 +162,12 @@ export default class DesktopCapturerSourcePicker extends React.Component - + ", () => { const defaultProps = { tabLocation: TabLocation.LEFT, tabs: [generalTab, labsTab, securityTab] as NonEmptyArray>, + onChange: () => {}, }; - const getComponent = (props = {}): React.ReactElement => ; + const getComponent = ( + props: { + activeTabId: "GENERAL" | "LABS" | "SECURITY"; + onChange?: () => any; + tabs?: NonEmptyArray>; + } = { + activeTabId: "GENERAL", + }, + ): React.ReactElement => ; const getTabTestId = (tab: Tab): string => `settings-tab-${tab.id}`; const getActiveTab = (container: HTMLElement): Element | undefined => @@ -42,38 +51,15 @@ describe("", () => { expect(container).toMatchSnapshot(); }); - it("renders first tab as active tab when no initialTabId", () => { - const { container } = render(getComponent()); - expect(getActiveTab(container)?.textContent).toEqual(_t(generalTab.label)); - expect(getActiveTabBody(container)?.textContent).toEqual("general"); - }); - - it("renders first tab as active tab when initialTabId is not valid", () => { - const { container } = render(getComponent({ initialTabId: "bad-tab-id" })); - expect(getActiveTab(container)?.textContent).toEqual(_t(generalTab.label)); - expect(getActiveTabBody(container)?.textContent).toEqual("general"); - }); - - it("renders initialTabId tab as active when valid", () => { - const { container } = render(getComponent({ initialTabId: securityTab.id })); - expect(getActiveTab(container)?.textContent).toEqual(_t(securityTab.label)); - expect(getActiveTabBody(container)?.textContent).toEqual("security"); - }); - - it("sets active tab on tab click", () => { - const { container, getByTestId } = render(getComponent()); - - act(() => { - fireEvent.click(getByTestId(getTabTestId(securityTab))); - }); - + it("renders activeTabId tab as active when valid", () => { + const { container } = render(getComponent({ activeTabId: securityTab.id })); expect(getActiveTab(container)?.textContent).toEqual(_t(securityTab.label)); expect(getActiveTabBody(container)?.textContent).toEqual("security"); }); it("calls onchange on on tab click", () => { const onChange = jest.fn(); - const { getByTestId } = render(getComponent({ onChange })); + const { getByTestId } = render(getComponent({ activeTabId: "GENERAL", onChange })); act(() => { fireEvent.click(getByTestId(getTabTestId(securityTab))); @@ -84,31 +70,13 @@ describe("", () => { it("keeps same tab active when order of tabs changes", () => { // start with middle tab active - const { container, rerender } = render(getComponent({ initialTabId: labsTab.id })); + const { container, rerender } = render(getComponent({ activeTabId: labsTab.id })); expect(getActiveTab(container)?.textContent).toEqual(_t(labsTab.label)); - rerender(getComponent({ tabs: [labsTab, generalTab, securityTab] })); + rerender(getComponent({ tabs: [labsTab, generalTab, securityTab], activeTabId: labsTab.id })); // labs tab still active expect(getActiveTab(container)?.textContent).toEqual(_t(labsTab.label)); }); - - it("does not reactivate inititalTabId on rerender", () => { - const { container, getByTestId, rerender } = render(getComponent()); - - expect(getActiveTab(container)?.textContent).toEqual(_t(generalTab.label)); - - // make security tab active - act(() => { - fireEvent.click(getByTestId(getTabTestId(securityTab))); - }); - expect(getActiveTab(container)?.textContent).toEqual(_t(securityTab.label)); - - // rerender with new tab location - rerender(getComponent({ tabLocation: TabLocation.TOP })); - - // still security tab - expect(getActiveTab(container)?.textContent).toEqual(_t(securityTab.label)); - }); }); diff --git a/test/components/views/elements/DesktopCapturerSourcePicker-test.tsx b/test/components/views/elements/DesktopCapturerSourcePicker-test.tsx new file mode 100644 index 0000000000..aafa356a3d --- /dev/null +++ b/test/components/views/elements/DesktopCapturerSourcePicker-test.tsx @@ -0,0 +1,100 @@ +/* +Copyright 2024 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import React from "react"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; + +import DesktopCapturerSourcePicker from "../../../../src/components/views/elements/DesktopCapturerSourcePicker"; +import PlatformPeg from "../../../../src/PlatformPeg"; +import BasePlatform from "../../../../src/BasePlatform"; + +const SOURCES = [ + { + id: "screen1", + name: "Screen 1", + thumbnailURL: "data:image/png;base64,", + }, + { + id: "window1", + name: "Window 1", + thumbnailURL: "data:image/png;base64,", + }, +]; + +describe("DesktopCapturerSourcePicker", () => { + beforeEach(() => { + const plaf = { + getDesktopCapturerSources: jest.fn().mockResolvedValue(SOURCES), + supportsSetting: jest.fn().mockReturnValue(false), + }; + jest.spyOn(PlatformPeg, "get").mockReturnValue(plaf as unknown as BasePlatform); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it("should render the component", () => { + render( {}} />); + expect(screen.getByRole("button", { name: "Cancel" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Share" })).toBeInTheDocument(); + }); + + it("should disable share button until a source is selected", () => { + render( {}} />); + expect(screen.getByRole("button", { name: "Share" })).toBeDisabled(); + }); + + it("should contain a screen source in the default tab", async () => { + render( {}} />); + + const screen1Button = await screen.findByRole("button", { name: "Screen 1" }); + + expect(screen1Button).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Window 1" })).not.toBeInTheDocument(); + }); + + it("should contain a window source in the window tab", async () => { + render( {}} />); + + await userEvent.click(screen.getByRole("tab", { name: "Application window" })); + + const window1Button = await screen.findByRole("button", { name: "Window 1" }); + + expect(window1Button).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Screen 1" })).not.toBeInTheDocument(); + }); + + it("should call onFinished with no arguments if cancelled", async () => { + const onFinished = jest.fn(); + render(); + + await userEvent.click(screen.getByRole("button", { name: "Cancel" })); + expect(onFinished).toHaveBeenCalledWith(); + }); + + it("should call onFinished with the selected source when share clicked", async () => { + const onFinished = jest.fn(); + render(); + + const screen1Button = await screen.findByRole("button", { name: "Screen 1" }); + + await userEvent.click(screen1Button); + await userEvent.click(screen.getByRole("button", { name: "Share" })); + expect(onFinished).toHaveBeenCalledWith(SOURCES[0]); + }); +});