Skip to content

Commit

Permalink
Components: require explicit children prop for all components (#31817)
Browse files Browse the repository at this point in the history
* Initial commit to require explicit children prop for all components

* Fix mispelled prop declaration

* Add pending TODO items

* Remove comments

* Clean up code around removing children props from root component types

* Disallow Divider component from having children

* Remove TODO comment

* Add explanation about `children` prop omission in ViewOwnProps

* Use `React.ElementType` instead of `As` from `reakit-utils`

* Add `children` prop to View

* Do not use `ComponentType` as it pulls in optional `children` prop implicitly

* Mark `children` prop in <VisuallyHidden /> as required

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
  • Loading branch information
ciampo and sarayourfriend authored May 26, 2021
1 parent f1f174c commit 50e78fc
Show file tree
Hide file tree
Showing 18 changed files with 75 additions and 23 deletions.
2 changes: 1 addition & 1 deletion packages/components/src/divider/component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import type { PolymorphicComponentProps } from '../ui/context';
import * as styles from './styles';
import { space } from '../ui/utils/space';

export interface DividerProps extends SeparatorProps {
export interface DividerProps extends Omit< SeparatorProps, 'children' > {
/**
* Adjusts all margins.
*/
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/elevation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,4 @@ export type Props = {
* In the example below, `isInteractive` is activated to give a better sense of depth.
*/
value?: number;
children?: never;
};
8 changes: 8 additions & 0 deletions packages/components/src/flex/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ export type FlexProps = {
* @default false
*/
wrap?: boolean;
/**
* The children elements.
*/
children: React.ReactNode;
/**
* @deprecated
*/
Expand All @@ -70,6 +74,10 @@ export type FlexItemProps = {
* @default true
*/
isBlock?: boolean;
/**
* The children elements.
*/
children: React.ReactNode;
};

export type FlexBlockProps = Omit< FlexItemProps, 'isBlock' >;
4 changes: 4 additions & 0 deletions packages/components/src/grid/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,8 @@ export type Props = {
* Adjusts the CSS grid `template-rows`.
*/
templateRows?: CSSProperties[ 'gridTemplateRows' ];
/**
* The children elements.
*/
children: React.ReactNode;
};
4 changes: 4 additions & 0 deletions packages/components/src/spacer/hook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ export interface SpacerProps {
* Adjusts right padding.
*/
paddingRight?: number;
/**
* The children elements.
*/
children?: React.ReactNode;
}

export function useSpacer(
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/truncate/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ export interface Props {
*/
limit?: number;
/**
* Clamps the text content to the specifiec `numberOfLines`, adding the `ellipsis` at the end.
* Clamps the text content to the specified `numberOfLines`, adding the `ellipsis` at the end.
*/
numberOfLines?: number;
/**
* The children elements.
*/
children: React.ReactNode;
}
8 changes: 8 additions & 0 deletions packages/components/src/ui/card/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ export type CardBodyProps = {
* @default true
*/
scrollable?: boolean;
/**
* The children elements.
*/
children: React.ReactNode;
};

export type CardHeaderSize = 'medium' | 'small' | 'xSmall';
Expand All @@ -62,6 +66,10 @@ export type CardHeaderProps = {
* @default 'medium'
*/
size?: CardHeaderSize;
/**
* The children elements.
*/
children: React.ReactNode;
};

export type CardFooterProps = CardHeaderProps & {
Expand Down
21 changes: 13 additions & 8 deletions packages/components/src/ui/context/polymorphic-component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,18 @@
*/
// eslint-disable-next-line no-restricted-imports
import type * as React from 'react';
import type { As, RenderProp, ExtractHTMLAttributes } from 'reakit-utils/types';
import type { Interpolation } from 'create-emotion';

/**
* Based on https://github.com/reakit/reakit/blob/master/packages/reakit-utils/src/types.ts
*
* The `children` prop is being explicitely omitted since it is otherwise implicitly added
* by `ComponentPropsWithRef`. The context is that components should require the `children`
* prop explicitely when needed (see https://github.com/WordPress/gutenberg/pull/31817).
*/
export type PolymorphicComponentProps< P, T extends As > = P &
Omit< React.ComponentPropsWithRef< T >, 'as' | keyof P > & {
export type PolymorphicComponentProps< P, T extends React.ElementType > = P &
Omit< React.ComponentPropsWithRef< T >, 'as' | keyof P | 'children' > & {
as?: T | keyof JSX.IntrinsicElements;
children?: React.ReactNode | RenderProp< ExtractHTMLAttributes< any > >;
};

export type ElementTypeFromPolymorphicComponentProps<
Expand All @@ -23,8 +25,8 @@ export type PropsFromPolymorphicComponentProps<
P
> = P extends PolymorphicComponentProps< infer PP, any > ? PP : never;

export type PolymorphicComponent< T extends As, O > = {
< TT extends As >(
export type PolymorphicComponent< T extends React.ElementType, O > = {
< TT extends React.ElementType >(
props: PolymorphicComponentProps< O, TT > & { as: TT }
): JSX.Element | null;
( props: PolymorphicComponentProps< O, T > ): JSX.Element | null;
Expand All @@ -40,7 +42,7 @@ export type PolymorphicComponent< T extends As, O > = {
selector: `.${ string }`;
};

export type CreatePolymorphicComponent< T extends As, P > = (
export type CreatePolymorphicComponent< T extends React.ElementType, P > = (
template: TemplateStringsArray,
...styles: (
| Interpolation< undefined >
Expand All @@ -62,5 +64,8 @@ type CreateStyledComponents = {
};

export type CreateStyled = CreateStyledComponents & {
< T extends As >( component: T ): CreatePolymorphicComponent< T, {} >;
< T extends React.ElementType >( component: T ): CreatePolymorphicComponent<
T,
{}
>;
};
5 changes: 2 additions & 3 deletions packages/components/src/ui/context/use-context-system.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,10 @@ import { useComponentsContext } from './context-system-provider';
import { getNamespace, getConnectedNamespace } from './utils';
import { getStyledClassNameFromKey } from './get-styled-class-name-from-key';

/* eslint-disable jsdoc/valid-types */
/**
* @template TProps
* @typedef {TProps & { className: string; children?: import('react').ReactNode }} ConnectedProps
* @typedef {TProps & { className: string; }} ConnectedProps
*/
/* eslint-enable jsdoc/valid-types */

/**
* Custom hook that derives registered props from the Context system.
Expand Down Expand Up @@ -75,6 +73,7 @@ export function useContextSystem( props, namespace ) {
finalComponentProps[ key ] = overrideProps[ key ];
}

// @ts-ignore
finalComponentProps.children = rendered;
finalComponentProps.className = classes;

Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/ui/context/with-next.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { useContextSystem } from './use-context-system';
* @template {{}} TCurrentProps
* @template {{}} TNextProps
* @param {import('react').ForwardRefExoticComponent<TCurrentProps>} CurrentComponent
* @param {import('react').ComponentType<TNextProps>} NextComponent
* @param {(props: TNextProps) => JSX.Element | null} NextComponent
* @param {string} namespace
* @param {(props: TCurrentProps) => TNextProps} adapter
*/
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/ui/control-group/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type { FlexProps } from '../../flex/types';
export type ControlGroupContext = {
isFirst?: boolean;
isLast?: boolean;
isMidde?: boolean;
isMiddle?: boolean;
isOnly?: boolean;
isVertical?: boolean;
styles?: string;
Expand All @@ -23,4 +23,8 @@ export type Props = Pick< FlexProps, 'direction' > & {
* Adjust the layout (width) of content using CSS grid (`grid-template-columns`).
*/
templateColumns?: CSSProperties[ 'gridTemplateColumns' ];
/**
* The children elements.
*/
children: React.ReactNode;
};
5 changes: 5 additions & 0 deletions packages/components/src/ui/popover/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,14 @@ export type Props = PopperProps & {
* @see https://reakit.io/docs/popover/#usepopoverstate
*/
visible?: boolean;
/**
* The children elements.
*/
children: React.ReactNode;
};

export type ContentProps = {
elevation?: number;
maxWidth?: CSSProperties[ 'maxWidth' ];
children: React.ReactNode;
};
4 changes: 4 additions & 0 deletions packages/components/src/ui/scrollable/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@ export type Props = {
* @default false
*/
smoothScroll?: boolean;
/**
* The children elements.
*/
children: React.ReactNode;
};
4 changes: 4 additions & 0 deletions packages/components/src/ui/surface/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,8 @@ export type Props = {
* * `tertiary`: Used as the app/site wide background. Visible in **dark mode** only. Use case is rare.
*/
variant?: SurfaceVariant;
/**
* The children elements.
*/
children: React.ReactNode;
};
2 changes: 1 addition & 1 deletion packages/components/src/ui/tooltip/content.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const { TooltipPopoverView } = styles;

/**
*
* @param {import('../context').PolymorphicComponentProps<import('reakit').TooltipProps, 'div'>} props
* @param {import('../context').PolymorphicComponentProps<import('./types').ContentProps, 'div'>} props
* @param {import('react').Ref<any>} forwardedRef
*/
function TooltipContent( props, forwardedRef ) {
Expand Down
10 changes: 7 additions & 3 deletions packages/components/src/ui/tooltip/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
* External dependencies
*/
// eslint-disable-next-line no-restricted-imports
import type { TooltipInitialState } from 'reakit';
import type { TooltipInitialState, TooltipProps } from 'reakit';
// eslint-disable-next-line no-restricted-imports
import type { ReactElement } from 'react';
import type { ReactElement, ReactNode } from 'react';

/**
* Internal dependencies
Expand Down Expand Up @@ -59,6 +59,10 @@ export type Props = TooltipInitialState &
* @see https://reakit.io/docs/tooltip/#usetooltipstate
*/
visible?: boolean;
children: JSX.Element;
children: ReactElement;
focusable?: boolean;
};

export type ContentProps = TooltipProps & {
children: ReactNode;
};
4 changes: 2 additions & 2 deletions packages/components/src/ui/visually-hidden/hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import { cx } from 'emotion';
import * as styles from './styles';

// duplicate this for the sake of being able to export it, it'll be removed when we replace VisuallyHidden in components/src anyway
/** @typedef {import('../context').PolymorphicComponentProps<{}, 'div'>} Props */
/** @typedef {import('../context').PolymorphicComponentProps<{ children: import('react').ReactNode }, 'div'>} Props */

/**
* @param {import('../context').PolymorphicComponentProps<{}, 'div'>} props
* @param {import('../context').PolymorphicComponentProps<{ children: import('react').ReactNode }, 'div'>} props
*/
export function useVisuallyHidden( { className, ...props } ) {
// circumvent the context system and write the classnames ourselves
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/view/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import styled from '@emotion/styled';
* }
* ```
*
* @type {import('../ui/context').PolymorphicComponent<'div', {}>}
* @type {import('../ui/context').PolymorphicComponent<'div', { children?: import('react').ReactNode }>}
*/
// @ts-ignore
const View = styled.div``;
Expand Down

0 comments on commit 50e78fc

Please sign in to comment.