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

Panel: Convert to TypeScript #47259

Merged
merged 10 commits into from
Feb 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

- `Toolbar`: unify Storybook examples under one file, migrate from knobs to controls ([47117](https://github.com/WordPress/gutenberg/pull/47117)).
- `DropdownMenu`: migrate Storybook to controls ([47149](https://github.com/WordPress/gutenberg/pull/47149)).
- `Panel`, `PanelHeader`, `PanelRow`: Convert to TypeScript ([#47259](https://github.com/WordPress/gutenberg/pull/47259)).
- Removed deprecated `@storybook/addon-knobs` dependency from the package ([47152](https://github.com/WordPress/gutenberg/pull/47152)).
- `ColorListPicker`: Convert to TypeScript ([#46358](https://github.com/WordPress/gutenberg/pull/46358)).
- `KeyboardShortcuts`: Convert to TypeScript ([#47429](https://github.com/WordPress/gutenberg/pull/47429)).
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/panel/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ Props that are passed to the `Button` component in the `PanelBodyTitle` within t

#### PanelRow

The `PanelRow` is a generic container for panel content. Default styles add a top margin and arrange items in a flex row.
`PanelRow` is a generic container for rows within a `PanelBody`. It is a flex container with a top margin for spacing.

##### Props

Expand All @@ -182,7 +182,7 @@ PanelRow accepts a forwarded ref that will be added to the wrapper div. Usage:

#### PanelHeader

This is a simple container for a header component. This is used by the `Panel` component under the hood, so it does not typically need to be used.
`PanelHeader` renders the header for the `Panel`. This is used by the `Panel` component under the hood, so it does not typically need to be used.

##### Props

Expand Down
10 changes: 0 additions & 10 deletions packages/components/src/panel/header.js

This file was deleted.

20 changes: 20 additions & 0 deletions packages/components/src/panel/header.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Internal dependencies
*/
import type { PanelHeaderProps } from './types';

/**
* `PanelHeader` renders the header for the `Panel`.
* This is used by the `Panel` component under the hood,
* so it does not typically need to be used.
*/
function PanelHeader( { label, children }: PanelHeaderProps ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not going to add PanelHeader the Storybook for now because it shouldn't really be used independently. No usages in the repo either. If it weren't for back compat I would remove it from the public exports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider deprecating it to discourage further usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is, I doubt anyone is using it because there is no reason to πŸ˜† So it's not that I want to discourage people from using it per se, it's more that I don't want to clutter our docs with something that is irrelevant to 99.9% of people.

return (
<div className="components-panel__header">
{ label && <h2>{ label }</h2> }
{ children }
</div>
);
}

export default PanelHeader;
26 changes: 0 additions & 26 deletions packages/components/src/panel/index.js

This file was deleted.

48 changes: 48 additions & 0 deletions packages/components/src/panel/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { forwardRef } from '@wordpress/element';

/**
* Internal dependencies
*/
import PanelHeader from './header';
import type { PanelProps } from './types';

function UnforwardedPanel(
{ header, className, children }: PanelProps,
ref: React.ForwardedRef< HTMLDivElement >
) {
const classNames = classnames( className, 'components-panel' );
return (
<div className={ classNames } ref={ ref }>
{ header && <PanelHeader label={ header } /> }
{ children }
</div>
);
}

/**
* `Panel` expands and collapses multiple sections of content.
*
* ```jsx
* import { Panel, PanelBody, PanelRow } from '@wordpress/components';
* import { more } from '@wordpress/icons';
*
* const MyPanel = () => (
* <Panel header="My Panel">
* <PanelBody title="My Block Settings" icon={ more } initialOpen={ true }>
* <PanelRow>My Panel Inputs and Labels</PanelRow>
* </PanelBody>
* </Panel>
* );
* ```
*/
export const Panel = forwardRef( UnforwardedPanel );

export default Panel;
20 changes: 0 additions & 20 deletions packages/components/src/panel/row.js

This file was deleted.

37 changes: 37 additions & 0 deletions packages/components/src/panel/row.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { forwardRef } from '@wordpress/element';
import type { ForwardedRef } from 'react';

/**
* Internal dependencies
*/
import type { PanelRowProps } from './types';

function UnforwardedPanelRow(
{ className, children }: PanelRowProps,
ref: ForwardedRef< HTMLDivElement >
) {
return (
<div
className={ classnames( 'components-panel__row', className ) }
ref={ ref }
>
{ children }
</div>
);
}

/**
* `PanelRow` is a generic container for rows within a `PanelBody`.
* It is a flex container with a top margin for spacing.
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that these info can be useful for a dev using this component, but should we expose implementation details like layout APIs (flex) and design details (margin top) ? An interesting dilemma, I personally don't have a strong opinion

Copy link
Member Author

Choose a reason for hiding this comment

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

For this one I decided it was necessary to mention because it's essential to what PanelRow is. Those two styles are the only reason the component exists, and I phrased it in a way that we can stick to as a contract. In other words, if it ceases to be a flex container, or if it ceases to have a top margin, I would consider it a breaking change.

*/
export const PanelRow = forwardRef( UnforwardedPanelRow );

export default PanelRow;
22 changes: 22 additions & 0 deletions packages/components/src/panel/stories/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import Panel from '../';
import PanelRow from '../row';
import PanelBody from '../body';
import InputControl from '../../input-control';

/**
* WordPress dependencies
Expand Down Expand Up @@ -56,6 +57,27 @@ Default.args = {
),
};

/**
* `PanelRow` is a generic container for rows within a `PanelBody`.
* It is a flex container with a top margin for spacing.
*/
export const _PanelRow = Template.bind( {} );
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this story to clarify what the default styling of PanelRow looks like. I don't quite understand why it is space-between, but that's something we'll have to keep for back compat. Also the 8px margin-top seems way too narrow.

Though I'm wondering whether we can somehow salvage and update this component to use as #43423 πŸ€” Something to think about!

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! I think that Panel overall needs a refresh (or a deprecation), especially since more and more of our UI uses ToolsPanel. Something that we could at some point discuss with @jasmussen

_PanelRow.args = {
children: (
<PanelBody title="My Profile">
<PanelRow>
<InputControl label="First name" />
<InputControl label="Last name" />
</PanelRow>
<PanelRow>
<div style={ { flex: 1 } }>
<InputControl label="Email" />
</div>
</PanelRow>
</PanelBody>
),
};

export const DisabledSection = Template.bind( {} );
DisabledSection.args = {
...Default.args,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { render, screen } from '@testing-library/react';
/**
* Internal dependencies
*/
import PanelHeader from '../header.js';
import PanelHeader from '../header';

describe( 'PanelHeader', () => {
describe( 'basic rendering', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,28 @@ import { render, screen } from '@testing-library/react';
/**
* Internal dependencies
*/
import Panel from '../';
import Panel from '..';

describe( 'Panel', () => {
describe( 'basic rendering', () => {
it( 'should render an empty div without any provided props', () => {
const { container } = render( <Panel /> );
const { container } = render( <Panel children={ null } /> );

expect( container ).toMatchSnapshot();
} );

it( 'should render a heading when provided text in the header prop', () => {
render( <Panel header="Header Label" /> );
render( <Panel header="Header Label" children={ null } /> );

const heading = screen.getByRole( 'heading' );
expect( heading ).toBeVisible();
expect( heading ).toHaveTextContent( 'Header Label' );
} );

it( 'should render an additional className', () => {
const { container } = render( <Panel className="the-panel" /> );
const { container } = render(
<Panel className="the-panel" children={ null } />
);

expect( container ).toMatchSnapshot();
} );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ import PanelRow from '../row';

describe( 'PanelRow', () => {
it( 'should render with the default class name', () => {
const { container } = render( <PanelRow /> );
const { container } = render( <PanelRow children={ null } /> );

expect( container ).toMatchSnapshot();
} );

it( 'should render with the custom class name', () => {
const { container } = render( <PanelRow className="custom" /> );
const { container } = render(
<PanelRow className="custom" children={ null } />
);

expect( container ).toMatchSnapshot();
} );
Expand Down
38 changes: 38 additions & 0 deletions packages/components/src/panel/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
export type PanelProps = {
/**
* The text that will be rendered as the title of the panel.
* Text will be rendered inside an `<h2>` tag.
*/
header?: PanelHeaderProps[ 'label' ];
/**
* The CSS class to apply to the wrapper element.
*/
className?: string;
/**
* The content to display within the panel.
*/
children: React.ReactNode;
};

export type PanelHeaderProps = {
/**
* The text that will be rendered as the title of the panel.
* Will be rendered in an `<h2>` tag.
*/
label?: string;
/**
* The content to display within the panel header.
*/
children?: React.ReactNode;
};

export type PanelRowProps = {
/**
* The CSS class to apply to the wrapper element.
*/
className?: string;
/**
* The content to display within the panel row.
*/
children: React.ReactNode;
};
2 changes: 1 addition & 1 deletion packages/components/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"src/menu-items-choice",
"src/navigation",
"src/palette-edit",
"src/panel",
"src/panel/body.js",
"src/toolbar",
"src/tree-grid"
]
Expand Down