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: add typings for <Button> #3080

Merged
merged 3 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
13 changes: 8 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@
"@testing-library/react-hooks": "^8.0.1",
"@testing-library/user-event": "^13.5.0",
"@types/jest": "^29.5.10",
"@types/react": "17.0.0",
"@types/react-dom": "17.0.11",
"@types/react": "^17.0.80",
"@types/react-dom": "^17.0.11",
"@types/react-responsive": "^8.0.8",
"@types/react-table": "^7.7.19",
"@types/react-test-renderer": "^18.0.0",
Expand Down
16 changes: 14 additions & 2 deletions src/Button/Button.test.jsx → src/Button/Button.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('<Button />', () => {

it('renders with props iconAfter and size', () => {
const tree = renderer.create((
<Button iconAfter={Close} size="md">Button</Button>
<Button iconAfter={Close} size="sm">Button</Button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed this to sm for some variation - too many of the tests were just using the same md size.

)).toJSON();
expect(tree).toMatchSnapshot();
});
Expand Down Expand Up @@ -94,9 +94,21 @@ describe('<Button />', () => {
});

test('test button as hyperlink', () => {
render(<Button as={Hyperlink} destination="https://www.poop.com/💩">Button</Button>);
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const ref = (_current: HTMLAnchorElement) => {}; // Check typing of a ref - should not show type errors.
render(<Button as={Hyperlink} ref={ref} destination="https://www.poop.com/💩">Button</Button>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shows an example of how passing as={Hyperlink} changes the types of various other props like ref to match.

expect(screen.getByRole('link').getAttribute('href')).toEqual('https://www.poop.com/💩');
});
});

test('with size="inline"', () => {
const tree = renderer.create((
<p>
<span className="mr-1">2 items selected.</span>
<Button variant="link" size="inline">Clear</Button>
</p>
)).toJSON();
expect(tree).toMatchSnapshot();
});
});
});
File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ exports[`<Button /> correct rendering renders with props iconAfter 1`] = `

exports[`<Button /> correct rendering renders with props iconAfter and size 1`] = `
<button
className="btn btn-primary btn-md"
className="btn btn-primary btn-sm"
disabled={false}
type="button"
>
Button
<span
className="pgn__icon pgn__icon__md btn-icon-after"
className="pgn__icon pgn__icon__sm btn-icon-after"
>
<svg
aria-hidden={true}
Expand Down Expand Up @@ -197,3 +197,20 @@ exports[`<Button /> correct rendering renders without props 1`] = `
Button
</button>
`;

exports[`<Button /> correct rendering with size="inline" 1`] = `
<p>
<span
className="mr-1"
>
2 items selected.
</span>
<button
className="btn btn-link btn-inline"
disabled={false}
type="button"
>
Clear
</button>
</p>
`;
75 changes: 59 additions & 16 deletions src/Button/index.jsx → src/Button/index.tsx
Original file line number Diff line number Diff line change
@@ -1,32 +1,57 @@
import React from 'react';
import PropTypes from 'prop-types';
import PropTypes, { type Requireable } from 'prop-types';
import classNames from 'classnames';
import BaseButton from 'react-bootstrap/Button';
import BaseButtonGroup from 'react-bootstrap/ButtonGroup';
import BaseButtonToolbar from 'react-bootstrap/ButtonToolbar';
import BaseButton, { type ButtonProps as BaseButtonProps } from 'react-bootstrap/Button';
import BaseButtonGroup, { type ButtonGroupProps as BaseButtonGroupProps } from 'react-bootstrap/ButtonGroup';
import BaseButtonToolbar, { type ButtonToolbarProps } from 'react-bootstrap/ButtonToolbar';
import { type BsPrefixRefForwardingComponent as ComponentWithAsProp } from 'react-bootstrap/esm/helpers';
Copy link
Member

@adamstankiewicz adamstankiewicz May 29, 2024

Choose a reason for hiding this comment

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

[question] Given this pattern of importing BsPrefixRefForwardingComponent (and BsPrefixProps, see #3082), I wonder if there's a more common/global location to define these types when renaming them so each individual component doesn't need to do the same re-naming repeatedly (e.g., risking different naming across components).

For example, Hyperlink (in #3082) will also rely on this pattern so it'd be nice to only rename it in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good idea and I'm happy to move it somewhere shared. Do you have a place in mind or want me to make something up?

Copy link
Contributor

Choose a reason for hiding this comment

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

First thing that came to my mind was to rename https://github.com/openedx/paragon/tree/master/src/utils/propTypes to src/utils/types and then put it in there, but I'm very open to any other ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I didn't rename propTypes for now since I want to distinguish them from types (but feel free to do that later if you want). But otherwise I did this: 66f85cb

I took the opportunity to add lots of documentation to explain how these types work (since the Bootstrap types are pretty obscure), and added in some type-only tests to verify that my understanding of them is correct (and support any future refactors).

// @ts-ignore - we're not going to bother adding types for the deprecated button
import ButtonDeprecated from './deprecated';

import Icon from '../Icon';

const Button = React.forwardRef(({
interface ButtonProps extends Omit<BaseButtonProps, 'size'> {
/**
* An icon component to render. Example:
* ```
* import { Close } from '@openedx/paragon/icons';
* <Button iconBefore={Close}>Close</Button>
* ```
*/
iconBefore?: React.ComponentType;
/**
* An icon component to render. Example:
* ```
* import { Close } from '@openedx/paragon/icons';
* <Button iconAfter={Close}>Close</Button>
* ```
*/
iconAfter?: React.ComponentType;
size?: 'sm' | 'md' | 'lg' | 'inline';
}

type ButtonType = ComponentWithAsProp<'button', ButtonProps> & { Deprecated?: any };

const Button: ButtonType = React.forwardRef<HTMLButtonElement, ButtonProps>(({
children,
iconAfter,
iconBefore,
size,
...props
}, ref) => (
<BaseButton
size={size as 'sm' | 'lg' | undefined} // Bootstrap's <Button> types do not allow 'md' or 'inline', but we do.
{...props}
className={classNames(props.className)}
ref={ref}
>
{iconBefore && <Icon className="btn-icon-before" size={props.size} src={iconBefore} />}
{iconBefore && <Icon className="btn-icon-before" size={size} src={iconBefore} />}
{children}
{iconAfter && <Icon className="btn-icon-after" size={props.size} src={iconAfter} />}
{iconAfter && <Icon className="btn-icon-after" size={size} src={iconAfter} />}
</BaseButton>
));

Button.propTypes = {
...Button.propTypes,
/** Specifies class name to apply to the button */
className: PropTypes.string,
/** Disables the Button, preventing mouse events, even if the underlying component is an `<a>` element */
Expand All @@ -52,10 +77,13 @@ Button.propTypes = {
variant: PropTypes.string,
/** An icon component to render.
* Example import of a Paragon icon component: `import { Check } from '@openedx/paragon/icons';` */
iconBefore: PropTypes.oneOfType([PropTypes.elementType, PropTypes.node]),
iconBefore: PropTypes.elementType as Requireable<React.ComponentType>,
/** An icon component to render.
* Example import of a Paragon icon component: `import { Check } from '@openedx/paragon/icons';` */
iconAfter: PropTypes.oneOfType([PropTypes.elementType, PropTypes.node]),
iconAfter: PropTypes.elementType as Requireable<React.ComponentType>,
// The 'as' type casting above is required for TypeScript checking, because the 'PropTypes.elementType' type normally
// allows strings as a value (for use cases like 'div') but we don't support that for <Icon />/iconBefore/iconAfter.
// The React TypeScript type definitions are more specific (React.ComponentType vs React.ElementType).
};

Button.defaultProps = {
Expand All @@ -69,20 +97,29 @@ Button.defaultProps = {

Button.Deprecated = ButtonDeprecated;

function ButtonGroup(props) {
return <BaseButtonGroup {...props} />;
}
function ButtonToolbar(props) {
return <BaseButtonToolbar {...props} />;
// We could just re-export 'ButtonGroup' and 'ButtonToolbar', but we currently
// override them to add propTypes validation at runtime, since most Paragon
// consumers aren't using TypeScript yet. We also force ButtonGroup's 'size'
// prop to accept our custom values of 'md' and 'inline' which are used in
// Paragon but not used in the base Bootstrap classes.

interface ButtonGroupProps extends Omit<BaseButtonGroupProps, 'size'> {
size?: 'sm' | 'md' | 'lg' | 'inline';
}

const ButtonGroup: ComponentWithAsProp<'div', ButtonGroupProps> = (
React.forwardRef<HTMLButtonElement, ButtonGroupProps>(({ size, ...props }, ref) => (
<BaseButtonGroup size={size as 'sm' | 'lg'} {...props} ref={ref} />
))
);

ButtonGroup.propTypes = {
/** Specifies element type for this component. */
as: PropTypes.elementType,
/** An ARIA role describing the button group. */
role: PropTypes.string,
/** Specifies the size for all Buttons in the group. */
size: PropTypes.oneOf(['sm', 'md', 'lg']),
size: PropTypes.oneOf(['sm', 'md', 'lg', 'inline']),
/** Display as a button toggle group. */
toggle: PropTypes.bool,
/** Specifies if the set of Buttons should appear vertically stacked. */
Expand All @@ -100,6 +137,12 @@ ButtonGroup.defaultProps = {
size: 'md',
};

const ButtonToolbar: ComponentWithAsProp<'div', ButtonToolbarProps> = (
React.forwardRef<HTMLButtonElement, ButtonToolbarProps>((props, ref) => (
<BaseButtonToolbar {...props} ref={ref} />
))
);

ButtonToolbar.propTypes = {
/** An ARIA role describing the button group. */
role: PropTypes.string,
Expand Down
2 changes: 1 addition & 1 deletion src/Chip/ChipIcon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { STYLE_VARIANTS } from './constants';

export interface ChipIconProps {
className: string,
src: React.ReactElement | Function,
src: React.ComponentType,
onClick?: KeyboardEventHandler & MouseEventHandler,
alt?: string,
variant: string,
Expand Down
10 changes: 5 additions & 5 deletions src/Chip/index.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { ForwardedRef, KeyboardEventHandler, MouseEventHandler } from 'react';
import PropTypes from 'prop-types';
import PropTypes, { type Requireable } from 'prop-types';
import classNames from 'classnames';
// @ts-ignore
import { requiredWhen } from '../utils/propTypes';
Expand All @@ -15,9 +15,9 @@ export interface IChip {
onClick?: KeyboardEventHandler & MouseEventHandler,
className?: string,
variant?: string,
iconBefore?: React.ReactElement | Function,
iconBefore?: React.ComponentType,
iconBeforeAlt?: string,
iconAfter?: React.ReactElement | Function,
iconAfter?: React.ComponentType,
iconAfterAlt?: string,
onIconBeforeClick?: KeyboardEventHandler & MouseEventHandler,
onIconAfterClick?: KeyboardEventHandler & MouseEventHandler,
Expand Down Expand Up @@ -111,7 +111,7 @@ Chip.propTypes = {
*
* `import { Check } from '@openedx/paragon/icons';`
*/
iconBefore: PropTypes.oneOfType([PropTypes.element, PropTypes.func]),
iconBefore: PropTypes.elementType as Requireable<React.ComponentType>,
/** Specifies icon alt text. */
iconBeforeAlt: requiredWhen(PropTypes.string, ['iconBefore', 'onIconBeforeClick']),
/** A click handler for the `Chip` icon before. */
Expand All @@ -122,7 +122,7 @@ Chip.propTypes = {
*
* `import { Check } from '@openedx/paragon/icons';`
*/
iconAfter: PropTypes.oneOfType([PropTypes.element, PropTypes.func]),
iconAfter: PropTypes.elementType as Requireable<React.ComponentType>,
/** Specifies icon alt text. */
iconAfterAlt: requiredWhen(PropTypes.string, ['iconAfter', 'onIconAfterClick']),
/** A click handler for the `Chip` icon after. */
Expand Down
6 changes: 4 additions & 2 deletions src/Icon/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import React from 'react';

export interface IconProps extends React.ComponentPropsWithoutRef<'span'> {
src?: React.ReactElement | Function;
// Note: React.ComponentType is what we want here. React.ElementType would allow some element type strings like "div",
// but we only want to allow components like 'Add' (a specific icon component function/class)
src?: React.ComponentType;
svgAttrs?: {
'aria-label'?: string;
'aria-labelledby'?: string;
};
id?: string | null;
size?: 'xs' | 'sm' | 'md' | 'lg';
size?: 'xs' | 'sm' | 'md' | 'lg' | 'inline';
className?: string | string[];
hidden?: boolean;
screenReaderText?: React.ReactNode;
Expand Down
2 changes: 1 addition & 1 deletion src/Icon/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ Icon.propTypes = {
* An icon component to render.
* Example import of a Paragon icon component: `import { Check } from '@openedx/paragon/icons';`
*/
src: PropTypes.oneOfType([PropTypes.element, PropTypes.elementType]),
src: PropTypes.elementType,
/** HTML element attributes to pass through to the underlying svg element */
svgAttrs: PropTypes.shape({
'aria-label': PropTypes.string,
Expand Down
2 changes: 1 addition & 1 deletion src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// Things that have types
// // // // // // // // // // // // // // // // // // // // // // // // // // //
export { default as Bubble } from './Bubble';
export { default as Button, ButtonGroup, ButtonToolbar } from './Button';
export { default as Chip, CHIP_PGN_CLASS } from './Chip';
export { default as ChipCarousel } from './ChipCarousel';
export { default as Hyperlink, HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, HYPER_LINK_EXTERNAL_LINK_TITLE } from './Hyperlink';
Expand All @@ -21,7 +22,6 @@ export const Avatar: any; // from './Avatar';
export const AvatarButton: any; // from './AvatarButton';
export const Badge: any; // from './Badge';
export const Breadcrumb: any; // from './Breadcrumb';
export const Button: any, ButtonGroup: any, ButtonToolbar: any; // from './Button';
export const
Card: any,
CardColumns: any,
Expand Down
6 changes: 3 additions & 3 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
// To keep this file in sync with the .d.ts file, it's in the same order
// and each line number is the same
// Keep this file in sync with the .d.ts file (manually). It's in the same order
// and each line number is the same, to make it easier.

// // // // // // // // // // // // // // // // // // // // // // // // // // //
// Things that have types
// // // // // // // // // // // // // // // // // // // // // // // // // // //
export { default as Bubble } from './Bubble';
export { default as Button, ButtonGroup, ButtonToolbar } from './Button';
export { default as Chip, CHIP_PGN_CLASS } from './Chip';
export { default as ChipCarousel } from './ChipCarousel';
export { default as Hyperlink, HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, HYPER_LINK_EXTERNAL_LINK_TITLE } from './Hyperlink';
Expand All @@ -21,7 +22,6 @@ export { default as Avatar } from './Avatar';
export { default as AvatarButton } from './AvatarButton';
export { default as Badge } from './Badge';
export { default as Breadcrumb } from './Breadcrumb';
export { default as Button, ButtonGroup, ButtonToolbar } from './Button';
export {
default as Card,
CardColumns,
Expand Down
2 changes: 1 addition & 1 deletion www/src/components/PageEditBtn/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function PageEditBtn({ githubEditPath, isNavLink, ...props }: PageEditBtnProps)
<Button
size="sm"
as={Hyperlink}
destination={githubEditPath}
destination={githubEditPath ?? ''}
variant="tertiary"
onClick={handlePageEditBtnClick}
target="_blank"
Expand Down
2 changes: 1 addition & 1 deletion www/src/components/header/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export interface IHeaderProps {
function Header({ siteTitle, showMinimizedTitle }: IHeaderProps) {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const [isOpen, , close, toggle] = useToggle(false);
const [target, setTarget] = useState(null);
const [target, setTarget] = useState<HTMLButtonElement | null>(null);
const { openSettings } = useContext(SettingsContext);
const isMobile = useMediaQuery({ maxWidth: breakpoints.small.maxWidth });

Expand Down
2 changes: 1 addition & 1 deletion www/src/components/header/Navbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import Search from '../Search';
export interface INavbar {
siteTitle: string,
onMenuClick: () => boolean,
setTarget: React.Dispatch<React.SetStateAction<null>>,
setTarget: React.Dispatch<React.SetStateAction<HTMLButtonElement | null>>,
onSettingsClick: Function | undefined,
menuIsOpen?: boolean,
showMinimizedTitle?: boolean,
Expand Down
Loading