PrivateCoffee/element-web
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 3

Make TabbedView a controlled component (#12480)

Browse source 
* 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
This commit is contained in:
David Baker 2024-05-03 16:01:01 +01:00 committed by GitHub
parent 2f3c84f1f4
commit 9684dd5145
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 170 additions and 95 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

47
src/components/structures/TabbedView.tsx
View file

@ -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<T extends string> {
) {}
}
export function useActiveTabWithDefault<T extends string>(
tabs: NonEmptyArray<Tab<string>>,
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<T extends string>({ tab, isActive, onClick }: ITabLabelProps<T
interface IProps<T extends string> {
// An array of objects representign tabs that the tabbed view will display.
tabs: NonEmptyArray<Tab<T>>;
// 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<T extends string> {
export default function TabbedView<T extends string>(props: IProps<T>): JSX.Element {
const tabLocation = props.tabLocation ?? TabLocation.LEFT;
const [activeTabId, setActiveTabId] = React.useState<T>((): 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<T> | undefined => {
return props.tabs.find((tab) => tab.id === id);
};
/**
* Shows the given tab
* @param {Tab} tab the tab to show
*/
const setActiveTab = (tab: Tab<T>): 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) => (
<TabLabel
key={"tab_label_" + tab.id}
tab={tab}
isActive={tab.id === activeTabId}
onClick={() => setActiveTab(tab)}
isActive={tab.id === props.activeTabId}
onClick={() => props.onChange(tab.id)}
/>
));
const tab = getTabById(activeTabId);
const tab = getTabById(props.activeTabId);
const panel = tab ? <TabPanel tab={tab} /> : null;
const tabbedViewClasses = classNames({

2
src/components/views/dialogs/InviteDialog.tsx
View file

@ -1538,7 +1538,7 @@ export default class InviteDialog extends React.PureComponent<Props, IInviteDial
<React.Fragment>
<TabbedView
tabs={tabs}
initialTabId={this.state.currentTabId}
activeTabId={this.state.currentTabId}
tabLocation={TabLocation.TOP}
onChange={this.onTabChange}
/>

12
src/components/views/dialogs/RoomSettingsDialog.tsx
View file

@ -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<IProps, IState> {
@ -70,7 +71,7 @@ class RoomSettingsDialog extends React.Component<IProps, IState> {
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<IProps, IState> {
if (event.getType() === EventType.RoomJoinRules) this.forceUpdate();
};
private onTabChange = (tabId: RoomSettingsTab): void => {
this.setState({ activeTabId: tabId });
};
private getTabs(): NonEmptyArray<Tab<RoomSettingsTab>> {
const tabs: Tab<RoomSettingsTab>[] = [];
@ -246,8 +251,9 @@ class RoomSettingsDialog extends React.Component<IProps, IState> {
<div className="mx_SettingsDialog_content">
<TabbedView
tabs={this.getTabs()}
initialTabId={this.props.initialTabId}
activeTabId={this.state.activeTabId}
screenName="RoomSettings"
onChange={this.onTabChange}
/>
</div>
</BaseDialog>

5
src/components/views/dialogs/SpacePreferencesDialog.tsx
View file

@ -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<Pick<IProps, "space">> = ({ space
);
};
const SpacePreferencesDialog: React.FC<IProps> = ({ space, initialTabId, onFinished }) => {
const SpacePreferencesDialog: React.FC<IProps> = ({ space, onFinished }) => {
const tabs: NonEmptyArray<Tab<SpacePreferenceTab>> = [
new Tab(
SpacePreferenceTab.Appearance,
@ -90,7 +89,7 @@ const SpacePreferencesDialog: React.FC<IProps> = ({ space, initialTabId, onFinis
<RoomName room={space} />
</h4>
<div className="mx_SettingsDialog_content">
<TabbedView tabs={tabs} initialTabId={initialTabId} />
<TabbedView tabs={tabs} activeTabId={SpacePreferenceTab.Appearance} onChange={() => {}} />
</div>
</BaseDialog>
);

4
src/components/views/dialogs/SpaceSettingsDialog.tsx
View file

@ -82,6 +82,8 @@ const SpaceSettingsDialog: React.FC<IProps> = ({ matrixClient: cli, space, onFin
].filter(Boolean) as NonEmptyArray<Tab<SpaceSettingsTab>>;
}, [cli, space, onFinished]);
const [activeTabId, setActiveTabId] = React.useState(SpaceSettingsTab.General);
return (
<BaseDialog
title={_t("space_settings|title", { spaceName: space.name || _t("common|unnamed_space") })}
@ -91,7 +93,7 @@ const SpaceSettingsDialog: React.FC<IProps> = ({ matrixClient: cli, space, onFin
fixedWidth={false}
>
<div className="mx_SpaceSettingsDialog_content" id="mx_SpaceSettingsDialog">
<TabbedView tabs={tabs} />
<TabbedView tabs={tabs} activeTabId={activeTabId} onChange={setActiveTabId} />
</div>
</BaseDialog>
);

11
src/components/views/dialogs/UserSettingsDialog.tsx
View file

@ -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<Tab<UserTab>>;
};
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")}
>
<div className="mx_SettingsDialog_content">
<TabbedView tabs={getTabs()} initialTabId={props.initialTabId} screenName="UserSettings" />
<TabbedView
tabs={getTabs()}
activeTabId={activeTabId}
screenName="UserSettings"
onChange={setActiveTabId}
/>
</div>
</BaseDialog>
</SDKContext.Provider>

21
src/components/views/elements/DesktopCapturerSourcePicker.tsx
View file

@ -85,8 +85,6 @@ export interface PickerIProps {
onFinished(source?: DesktopCapturerSource): void;
}
type TabId = "screen" | "window";
export default class DesktopCapturerSourcePicker extends React.Component<PickerIProps, PickerIState> {
public interval?: number;
@ -127,15 +125,15 @@ export default class DesktopCapturerSourcePicker extends React.Component<PickerI
this.props.onFinished(this.state.selectedSource);
};
private onTabChange = (): void => {
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<TabId> {
private getTab(type: Tabs, label: TranslationKey): Tab<Tabs> {
const sources = this.state.sources
.filter((source) => source.id.startsWith(type))
.map((source) => {
@ -153,9 +151,9 @@ export default class DesktopCapturerSourcePicker extends React.Component<PickerI
}
public render(): React.ReactNode {
const tabs: NonEmptyArray<Tab<TabId>> = [
this.getTab("screen", _td("voip|screenshare_monitor")),
this.getTab("window", _td("voip|screenshare_window")),
const tabs: NonEmptyArray<Tab<Tabs>> = [
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<PickerI
onFinished={this.onCloseClick}
title={_t("voip|screenshare_title")}
>
<TabbedView tabs={tabs} tabLocation={TabLocation.TOP} onChange={this.onTabChange} />
<TabbedView
tabs={tabs}
tabLocation={TabLocation.TOP}
activeTabId={this.state.selectedTab}
onChange={this.onTabChange}
/>
<DialogButtons
primaryButton={_t("action|share")}
hasCancel={true}

1
src/utils/DialogOpener.ts
View file

@ -91,7 +91,6 @@ export class DialogOpener {
Modal.createDialog(
SpacePreferencesDialog,
{
initialTabId: payload.initalTabId,
space: payload.space,
},
undefined,

62
test/components/structures/TabbedView-test.tsx
View file

@ -28,8 +28,17 @@ describe("<TabbedView />", () => {
const defaultProps = {
tabLocation: TabLocation.LEFT,
tabs: [generalTab, labsTab, securityTab] as NonEmptyArray<Tab<any>>,
onChange: () => {},
};
const getComponent = (props = {}): React.ReactElement => <TabbedView {...defaultProps} {...props} />;
const getComponent = (
props: {
activeTabId: "GENERAL" | "LABS" | "SECURITY";
onChange?: () => any;
tabs?: NonEmptyArray<Tab<any>>;
} = {
activeTabId: "GENERAL",
},
): React.ReactElement => <TabbedView {...defaultProps} {...props} />;
const getTabTestId = (tab: Tab<string>): string => `settings-tab-${tab.id}`;
const getActiveTab = (container: HTMLElement): Element | undefined =>
@ -42,38 +51,15 @@ describe("<TabbedView />", () => {
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("<TabbedView />", () => {
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));
});
});

100
test/components/views/elements/DesktopCapturerSourcePicker-test.tsx Normal file
View file

@ -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(<DesktopCapturerSourcePicker onFinished={() => {}} />);
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(<DesktopCapturerSourcePicker onFinished={() => {}} />);
expect(screen.getByRole("button", { name: "Share" })).toBeDisabled();
});
it("should contain a screen source in the default tab", async () => {
render(<DesktopCapturerSourcePicker onFinished={() => {}} />);
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(<DesktopCapturerSourcePicker onFinished={() => {}} />);
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(<DesktopCapturerSourcePicker onFinished={onFinished} />);
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(<DesktopCapturerSourcePicker onFinished={onFinished} />);
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]);
});
});