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

feat: export TabPanel #6896

Merged
merged 37 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
73a0519
add changelog entry
bvandercar-vt Jun 25, 2024
4ac5b19
feat: tabPanel renderer
bvandercar-vt Jul 5, 2024
6d456b5
refactor: generateTabIds
bvandercar-vt Jul 5, 2024
316f4a5
remove rename
bvandercar-vt Jul 5, 2024
7d1be88
export type
bvandercar-vt Jul 5, 2024
83116ef
fix order
bvandercar-vt Jul 5, 2024
0a007a1
style: return undefined
bvandercar-vt Jul 5, 2024
45d761a
refactor: isVisible prop
bvandercar-vt Jul 5, 2024
bcdb95b
style: jsdocs
bvandercar-vt Jul 5, 2024
c6b3717
Merge branch 'develop' into bvandercar/tabPanel
bvandercar-vt Jul 5, 2024
087ac23
remove return undefined
bvandercar-vt Jul 5, 2024
c2aed65
move key
bvandercar-vt Jul 5, 2024
9adf944
test: improve tabPanels tests
bvandercar-vt Jul 8, 2024
5472c30
Merge branch 'bvandercar/test/tabs' into bvandercar/tabPanel
bvandercar-vt Jul 8, 2024
1f2d078
fix test
bvandercar-vt Jul 8, 2024
fdf8652
style: use const
bvandercar-vt Jul 8, 2024
5b91852
style: date
bvandercar-vt Jul 8, 2024
b773ae3
add example
bvandercar-vt Jul 8, 2024
75a6efc
example: use section
bvandercar-vt Jul 8, 2024
241696b
Merge branch 'develop' of https://github.com/bvandercar-vt/blueprint …
bvandercar-vt Jul 8, 2024
150687f
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 12, 2024
f8794af
Merge branch 'develop' into bvandercar/tabPanel
bvandercar-vt Jul 12, 2024
110923a
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 16, 2024
955d4f1
Delete packages/core/changelog/@unreleased/pr-6865.v2.yml
bvandercar-vt Jul 16, 2024
aabcc6b
revert reorder
bvandercar-vt Jul 16, 2024
dc3f729
Merge branch 'bvandercar/tabPanel' of https://github.com/bvandercar-v…
bvandercar-vt Jul 16, 2024
7881c58
require isHidden
bvandercar-vt Jul 16, 2024
8f202ae
return undefined if panel undefined
bvandercar-vt Jul 16, 2024
fc9d945
pick renderActiveTabPanelOnly prop
bvandercar-vt Jul 16, 2024
2254a18
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 23, 2024
3fbc070
Merge branch 'develop' into bvandercar/tabPanel
bvandercar-vt Jul 23, 2024
9f9c0b6
add TabPanel example
bvandercar-vt Jul 23, 2024
6b820bd
uncontrolled usage example
bvandercar-vt Jul 23, 2024
0bd052b
style: tabs example
bvandercar-vt Jul 23, 2024
72bfa73
fix test
bvandercar-vt Jul 23, 2024
12a665b
change isHidden to selectedTabId
bvandercar-vt Jul 25, 2024
01cb908
Update packages/core/src/components/tabs/tabPanel.tsx
evansjohnson Jul 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/core/src/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ export { CheckboxCard, type CheckboxCardProps } from "./control-card/checkboxCar
export { RadioCard, type RadioCardProps } from "./control-card/radioCard";
export { SwitchCard, type SwitchCardProps } from "./control-card/switchCard";
export { Tab, type TabId, type TabProps } from "./tabs/tab";
export { TabPanel, type TabPanelProps } from "./tabs/tabPanel";
// eslint-disable-next-line deprecation/deprecation
export { Tabs, type TabsProps, TabsExpander, Expander } from "./tabs/tabs";
export { CompoundTag, type CompoundTagProps } from "./tag/compoundTag";
Expand Down
13 changes: 12 additions & 1 deletion packages/core/src/components/tabs/tab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,17 @@ import type { TagProps } from "../tag/tag";

export type TabId = string | number;

export interface TabIdProps {
/**
* `id` prop of the tab title, and the `aria-labelledby` of the `TabPanel`.
*/
tabTitleId: string;
/**
* `id` prop of the `tabpanel`, and the `aria-controls` of the tab title.
*/
tabPanelId: string;
}

export interface TabProps extends Props, Omit<HTMLDivProps, "id" | "title" | "onClick"> {
/**
* Content of tab title, rendered in a list above the active panel.
Expand All @@ -51,7 +62,7 @@ export interface TabProps extends Props, Omit<HTMLDivProps, "id" | "title" | "on
* If omitted, no panel will be rendered for this tab.
* Can either be an element or a renderer.
*/
panel?: React.JSX.Element | ((props: { tabTitleId: string; tabPanelId: string }) => React.JSX.Element);
panel?: React.JSX.Element | ((props: TabIdProps) => React.JSX.Element);

/**
* Space-delimited string of class names applied to tab panel container.
Expand Down
61 changes: 61 additions & 0 deletions packages/core/src/components/tabs/tabPanel.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright 2024 Palantir Technologies, Inc. All rights reserved.
*
* 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 classNames from "classnames";
import * as React from "react";

import { AbstractPureComponent, Classes, Utils } from "../../common";

import { type TabProps } from "./tab";
import type { TabsProps } from "./tabs";
import { generateTabIds, type TabTitleProps } from "./tabTitle";

export interface TabPanelProps
extends Pick<TabProps, "className" | "id" | "panel">,
Pick<TabsProps, "renderActiveTabPanelOnly">,
Pick<TabTitleProps, "parentId"> {
/**
* Used for setting `aria-hidden` prop.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Used for setting `aria-hidden` prop.
*/
/**
* If true, will set required accessibility roles and set `display` to `none`.
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking maybe we just make this required if the expected usage is that one TabPanel is provided for each tab, like is done in the uncontrolled usage of Tabs. In your example you essentially share a TabPanel but I'm wondering if making this optional encourages that pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

isHidden: boolean;
}

/**
* Wraps the passed `panel`.
*/
export class TabPanel extends AbstractPureComponent<TabPanelProps> {
public render() {
const { className, id, parentId, panel, isHidden, renderActiveTabPanelOnly } = this.props;

if (panel === undefined || (renderActiveTabPanelOnly && isHidden)) {
return undefined;
}

const { tabTitleId, tabPanelId } = generateTabIds(parentId, id);

return (
<div
aria-labelledby={tabTitleId}
aria-hidden={isHidden}
className={classNames(Classes.TAB_PANEL, className)}
id={tabPanelId}
role="tabpanel"
>
{Utils.isFunction(panel) ? panel({ tabTitleId, tabPanelId }) : panel}
</div>
);
}
}
19 changes: 10 additions & 9 deletions packages/core/src/components/tabs/tabTitle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { DISPLAYNAME_PREFIX, removeNonHTMLProps } from "../../common/props";
import { Icon } from "../icon/icon";
import { Tag } from "../tag/tag";

import type { TabId, TabProps } from "./tab";
import type { TabId, TabIdProps, TabProps } from "./tab";

export interface TabTitleProps extends TabProps {
/** Optional contents. */
Expand Down Expand Up @@ -55,18 +55,20 @@ export class TabTitle extends AbstractPureComponent<TabTitleProps> {
tagProps,
...htmlProps
} = this.props;

const intent = selected ? Intent.PRIMARY : Intent.NONE;
const { tabPanelId, tabTitleId } = generateTabIds(parentId, id);

return (
<div
{...removeNonHTMLProps(htmlProps)}
aria-controls={generateTabPanelId(parentId, id)}
aria-controls={tabPanelId}
aria-disabled={disabled}
aria-expanded={selected}
aria-selected={selected}
className={classNames(Classes.TAB, className)}
data-tab-id={id}
id={generateTabTitleId(parentId, id)}
id={tabTitleId}
onClick={disabled ? undefined : this.handleClick}
role="tab"
tabIndex={disabled ? undefined : selected ? 0 : -1}
Expand All @@ -91,10 +93,9 @@ export class TabTitle extends AbstractPureComponent<TabTitleProps> {
private handleClick = (e: React.MouseEvent<HTMLElement>) => this.props.onClick(this.props.id, e);
}

export function generateTabPanelId(parentId: TabId, tabId: TabId) {
return `${Classes.TAB_PANEL}_${parentId}_${tabId}`;
}

export function generateTabTitleId(parentId: TabId, tabId: TabId) {
return `${Classes.TAB}-title_${parentId}_${tabId}`;
export function generateTabIds(parentId: TabId, tabId: TabId) {
return {
tabPanelId: `${Classes.TAB_PANEL}_${parentId}_${tabId}`,
tabTitleId: `${Classes.TAB}-title_${parentId}_${tabId}`,
} satisfies TabIdProps;
Comment on lines +96 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not too much this component does, but it hooks up some accessibility roles that are otherwise liable to not be set.

Bingo, exactly!

}
21 changes: 8 additions & 13 deletions packages/core/src/components/tabs/tabs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import * as React from "react";
import { AbstractPureComponent, Classes, DISPLAYNAME_PREFIX, type Props, Utils } from "../../common";

import { Tab, type TabId, type TabProps } from "./tab";
import { generateTabPanelId, generateTabTitleId, TabTitle } from "./tabTitle";
import { TabPanel } from "./tabPanel";
import { TabTitle } from "./tabTitle";

/**
* Component that may be inserted between any two children of `<Tabs>` to right-align all subsequent children.
Expand Down Expand Up @@ -330,20 +331,14 @@ export class Tabs extends AbstractPureComponent<TabsProps, TabsState> {
return undefined;
}

const tabTitleId = generateTabTitleId(this.props.id, id);
const tabPanelId = generateTabPanelId(this.props.id, id);

return (
<div
aria-labelledby={tabTitleId}
aria-hidden={id !== this.state.selectedTabId}
className={classNames(Classes.TAB_PANEL, className, panelClassName)}
id={tabPanelId}
<TabPanel
{...tab.props}
key={id}
role="tabpanel"
>
{Utils.isFunction(panel) ? panel({ tabTitleId, tabPanelId }) : panel}
</div>
className={classNames(className, panelClassName)}
isHidden={id !== this.state.selectedTabId}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since any consumer would need to set this same logic up I wonder if we should have the prop be selectedTabId instead so that users can't do weird things like have multiple tab panels showing

Copy link
Contributor Author

@bvandercar-vt bvandercar-vt Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh the logic may not always be the same. Take, for example, this case where isHidden is always false because there's always only 1 TabPanel being rendered:

import * as React from "react";
import { Tab, Tabs, TabPanel, type TabId } from "@blueprintjs/core";
...

const TABS_PARENT_ID = React.useId();
const [selectedTabId, setSelectedTabId] = React.useState<TabId>("Home");

<Tabs
    id={TABS_PARENT_ID}
    onChange={setSelectedTabId}
    selectedTabId={selectedTabId}
>
    <Tab id="Home" title="Home" />
    <Tab id="Files" title="Files" />
</Tabs>
...
<TabPanel
    id={selectedTabId}
    isHidden={false}
    parentId={TABS_PARENT_ID}
    panel={<p>The current panel id is: "{selectedTabId}"</p>}
/>

But then again, if we changed it to selectedTabId they could just have it be

<TabPanel
    id={selectedTabId}
    selectedTabId={selectedTabId}
    parentId={TABS_PARENT_ID}
    panel={<p>The current panel id is: "{selectedTabId}"</p>}
/>

I'm down for either, let me know which you like better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I think I'm leaning towards selectedTabId because then we control more of the implementation - I'm worried about trying to use the same TabPanel for multiple tabs causing issues down the line somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 12a665b

parentId={this.props.id}
/>
);
};

Expand Down
1 change: 1 addition & 0 deletions packages/core/test/isotest.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ describe("@blueprintjs/core isomorphic rendering", () => {
"ContextMenuTargetLegacy",
"Expander",
"HotkeysTarget",
"TabPanel",
],
},
);
Expand Down
17 changes: 12 additions & 5 deletions packages/core/test/tabs/tabsTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { spy } from "sinon";
import { Classes } from "../../src/common";
import { Tab } from "../../src/components/tabs/tab";
import { Tabs, type TabsProps, type TabsState } from "../../src/components/tabs/tabs";
import { generateTabPanelId, generateTabTitleId } from "../../src/components/tabs/tabTitle";
import { generateTabIds } from "../../src/components/tabs/tabTitle";

describe("<Tabs>", () => {
const ID = "tabsTests";
Expand Down Expand Up @@ -113,7 +113,10 @@ describe("<Tabs>", () => {
<Tab id="third" title="Third" className={tabClassName} panel={<Panel title="third" />} />,
</Tabs>,
);
assert.lengthOf(wrapper.find(`.${tabClassName}`), 9);
bvandercar-vt marked this conversation as resolved.
Show resolved Hide resolved
const NUM_TABS = 3;
assert.lengthOf(wrapper.find(TAB), NUM_TABS);
assert.lengthOf(wrapper.find(TAB_PANEL), NUM_TABS);
assert.lengthOf(wrapper.find(`.${tabClassName}`).hostNodes(), NUM_TABS * 2);
});

it("attaches panelClassName to panel container if set", () => {
Expand All @@ -126,17 +129,21 @@ describe("<Tabs>", () => {
<Tab id="third" title="Third" panel={<Panel title="third" />} />,
</Tabs>,
);
assert.lengthOf(wrapper.find(`.${panelClassName}`), 1);
const NUM_TABS = 3;
assert.lengthOf(wrapper.find(TAB), NUM_TABS);
assert.lengthOf(wrapper.find(TAB_PANEL), NUM_TABS);
assert.lengthOf(wrapper.find(`.${panelClassName}`).hostNodes(), 1);
});

it("passes correct tabTitleId and tabPanelId to panel renderer", () => {
const expectedIds = generateTabIds(ID, "first");
mount(
<Tabs id={ID}>
<Tab
id="first"
panel={({ tabTitleId, tabPanelId }) => {
assert.equal(tabTitleId, generateTabTitleId(ID, "first"));
assert.equal(tabPanelId, generateTabPanelId(ID, "first"));
assert.equal(tabTitleId, expectedIds.tabTitleId);
assert.equal(tabPanelId, expectedIds.tabPanelId);
return <Panel title="a" />;
}}
/>
Expand Down
70 changes: 43 additions & 27 deletions packages/docs-app/src/examples/core-examples/tabsExample.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
Switch,
Tab,
type TabId,
TabPanel,
evansjohnson marked this conversation as resolved.
Show resolved Hide resolved
Tabs,
TabsExpander,
} from "@blueprintjs/core";
Expand Down Expand Up @@ -105,37 +106,52 @@ export class TabsExample extends React.PureComponent<ExampleProps, TabsExampleSt
</>
);

const NAVBAR_PARENT_ID = "navbar";

return (
<Example className="docs-tabs-example" options={options} {...this.props}>
<H4>Tabs without panels, controlled mode</H4>
<Switch checked={this.state.fill} label="Fill height" onChange={this.toggleFill} />
<Navbar>
<Navbar.Group>
<Navbar.Heading>
Page: <strong>{this.state.navbarTabId}</strong>
</Navbar.Heading>
</Navbar.Group>
<Navbar.Group align={Alignment.RIGHT}>
<Tabs
animate={this.state.animate}
fill={this.state.fill}
id="navbar"
large={this.state.large}
onChange={this.handleNavbarTabChange}
selectedTabId={this.state.navbarTabId}
>
<Tab id="Home" title="Home" icon={this.state.showIcon ? "home" : undefined} />
<Tab id="Files" title="Files" icon={this.state.showIcon ? "folder-open" : undefined} />
<Tab
id="Builds"
title="Builds"
icon={this.state.showIcon ? "build" : undefined}
tagContent={this.state.showTags ? 4 : undefined}
tagProps={{ round: this.state.useRoundTags }}
/>
</Tabs>
</Navbar.Group>
</Navbar>
<div className={Classes.SECTION}>
<Navbar>
<Navbar.Group>
<Navbar.Heading>
Page: <strong>{this.state.navbarTabId}</strong>
</Navbar.Heading>
</Navbar.Group>
<Navbar.Group align={Alignment.RIGHT}>
<Tabs
animate={this.state.animate}
fill={this.state.fill}
id={NAVBAR_PARENT_ID}
large={this.state.large}
onChange={this.handleNavbarTabChange}
selectedTabId={this.state.navbarTabId}
>
<Tab id="Home" title="Home" icon={this.state.showIcon ? "home" : undefined} />
<Tab id="Files" title="Files" icon={this.state.showIcon ? "folder-open" : undefined} />
<Tab
id="Builds"
title="Builds"
icon={this.state.showIcon ? "build" : undefined}
tagContent={this.state.showTags ? 4 : undefined}
tagProps={{ round: this.state.useRoundTags }}
/>
</Tabs>
</Navbar.Group>
</Navbar>
<TabPanel
id={this.state.navbarTabId}
isHidden={false}
parentId={NAVBAR_PARENT_ID}
panel={
<>
<H4>Example panel: {this.state.navbarTabId}</H4>
<p>The current panel is: "{this.state.navbarTabId}"</p>
</>
}
/>
</div>
<Divider style={{ margin: "20px 0", width: "100%" }} />
<H4>Tabs with panels, uncontrolled mode</H4>
<Switch checked={this.state.vertical} label="Use vertical tabs" onChange={this.toggleVertical} />
Expand Down