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

initial draft of actionIcon prop for Card component #1479

Closed
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 .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.DS_Store
.eslintcache
.prettierrc
node_modules
npm-debug.log
coverage
Expand Down
7 changes: 7 additions & 0 deletions src/Card/Card.scss
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@
}
}

.pgn__card-close-container {
Copy link
Member

Choose a reason for hiding this comment

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

It may make sense to make the class name more generic here, e.g. .pgn__card__action-icon-container, since this could represent more than just a close icon button.

position: absolute;
z-index: 10;
top: .5rem;
inset-inline-end: .25rem;
}

.pgn__card-divider {
border-top: 1px solid $card-divider-bg;
height: 0;
Expand Down
81 changes: 81 additions & 0 deletions src/Card/CardActionIcon.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import { IconButton, Icon } from '..';
import { Close, MoreVert } from '../../icons';

const CardActionIcon = React.forwardRef(
({
className,
actionIcon,
onClick,
isActive,
variant,
}, ref) => {
if (actionIcon === 'overflow') {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I feel CardActionIcon could probably be implemented in a bit more flexible way, that would allow consumers to customize it for any IconButton icon for the intended use case.

I'm thinking we could treat CardActionIcon essentially as a pass-thru component to IconButton, but with the extra styles to correctly position it in the Card, where Card.ActionIcon would take pass (?) of its props down to the underlying IconButton, perhaps with restrictions on which variants are valid for Card.ActionIcon.

With Card.ActionIcon as a sub-component, in theory, it could then be wrapped / composed by OverlayTrigger to optionally add a tooltip/popover to it., if needed.

<Card>
  <Card.ActionIcon
    src={Close}
    alt="Close"
    variant="dark"
  />
</Card>

Just a thought. Curious to hear your thoughts on how to make this a bit more flexible to support more use cases (e.g., using different icons, changing the IconButton variant, opting in to using a ModalPopup or Tooltip alongside the Card.ActionIcon, etc.).

return (
<div
className={classNames(
className,
'd-flex flex-wrap pgn__card-close-container',
Copy link
Member

Choose a reason for hiding this comment

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

Inside of Paragon components, we should avoid using CSS utility classes from Bootstrap as they usually include !important which hinders theming of the component; should a consumer want to override the component styles, having !important here could make that difficult. See this ADR for more details.

)}
ref={ref}
>
<IconButton
src={MoreVert}
iconAs={Icon}
alt="See more"
Copy link
Member

Choose a reason for hiding this comment

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

Paragon support i118n now, so we should avoid hardcoding any English strings. Let's use react-intl here to render a translated message instead (docs).

onClick={onClick}
variant={variant}
invertColors={variant === 'dark'}
isActive={isActive}
className="mr-2"
Copy link
Member

Choose a reason for hiding this comment

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

Avoid CSS utility class

/>
</div>
);
}
return (
<div
className={classNames(
className,
'd-flex flex-wrap pgn__card-close-container',
Copy link
Member

Choose a reason for hiding this comment

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

Avoid CSS utility class

)}
ref={ref}
>
<IconButton
src={Close}
iconAs={Icon}
alt="Close"
onClick={onClick}
variant={variant}
invertColors={variant === 'dark'}
isActive={isActive}
className="mr-2"
Copy link
Member

Choose a reason for hiding this comment

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

Avoid CSS utility class

/>
</div>
);
},
);

CardActionIcon.propTypes = {
/** Specifies class name to append to the base element. */
className: PropTypes.string,
/** Options for which type of actionIcon the Card will use. */
actionIcon: PropTypes.oneOf(['overflow', 'dismiss']),
/** Click handler for the button */
onClick: PropTypes.func,
/** whether to show the `IconButton` in an active state, whose styling is distinct from default state */
isActive: PropTypes.bool,
/** The visual style of the dialog box */
variant: PropTypes.oneOf(['default', 'warning', 'danger', 'success', 'dark']),
};

CardActionIcon.defaultProps = {
className: undefined,
actionIcon: 'dismiss',
onClick: () => {},
isActive: false,
variant: 'dark',
};

export default CardActionIcon;
2 changes: 1 addition & 1 deletion src/Card/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ This component uses a `Card` from react-bootstrap as a base component and extend
const isExtraSmall = useMediaQuery({ maxWidth: breakpoints.extraSmall.maxWidth });

return (
<Card style={{ width: isExtraSmall ? "100%" : "18rem" }}>
<Card actionIcon="dismiss" style={{ width: isExtraSmall ? "100%" : "18rem" }}>
<Card.ImageCap
src="https://source.unsplash.com/360x200/?nature,flower"
srcAlt="Card image"
Expand Down
12 changes: 11 additions & 1 deletion src/Card/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ import CardSection from './CardSection';
import CardFooter from './CardFooter';
import CardImageCap from './CardImageCap';
import CardBody from './CardBody';
import CardActionIcon from './CardActionIcon';

const Card = React.forwardRef(({
orientation,
actionIcon,
isLoading,
className,
isClickable,
muted,
children,
...props
}, ref) => (
<CardContextProvider orientation={orientation} isLoading={isLoading}>
Expand All @@ -28,7 +31,10 @@ const Card = React.forwardRef(({
})}
ref={ref}
tabIndex={isClickable ? '0' : '-1'}
/>
>
{actionIcon ? <CardActionIcon actionIcon={actionIcon} variant="dark" /> : undefined}
Copy link
Member

Choose a reason for hiding this comment

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

[curious] I'm wondering if it would make sense to have the consumer render a Card.ActionIcon sub-component in the children of Card to render this component, rather than relying on an actionIcon prop at the top-level Card component to be more in line with the other sub-components part of Card, e.g.

<Card>
  <Card.ActionIcon />
</Card>

{children}
</BaseCard>
</CardContextProvider>
));

Expand All @@ -37,6 +43,7 @@ export { default as CardDeck } from 'react-bootstrap/CardDeck';
export { default as CardImg } from 'react-bootstrap/CardImg';
export { default as CardGroup } from 'react-bootstrap/CardGroup';
export { default as CardGrid } from './CardGrid';
export { default as CardActionIcon } from './CardActionIcon';

Card.propTypes = {
...BaseCard.propTypes,
Expand All @@ -46,6 +53,8 @@ Card.propTypes = {
orientation: PropTypes.oneOf(['vertical', 'horizontal']),
/** Specifies whether the `Card` is clickable, if `true` appropriate `hover` and `focus` styling will be added. */
isClickable: PropTypes.bool,
/** Optional interactive icon positioned in the top right of the card. */
actionIcon: PropTypes.oneOf(['overflow', 'dismiss']),
/** Specifies loading state. */
isLoading: PropTypes.bool,
/** Specifies whether to display `Card` in muted styling. */
Expand All @@ -58,6 +67,7 @@ Card.defaultProps = {
orientation: 'vertical',
isClickable: false,
muted: false,
actionIcon: undefined,
isLoading: false,
};

Expand Down
18 changes: 18 additions & 0 deletions src/Card/tests/CardActionIcon.test.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from 'react';
import renderer from 'react-test-renderer';
import CardActionIcon from '../CardActionIcon';

describe('<CardActionIcon />', () => {
it('renders without props', () => {
const tree = renderer.create((
<CardActionIcon />
)).toJSON();
expect(tree).toMatchSnapshot();
});
it('renders with "overflow" option for actionIcon prop', () => {
const tree = renderer.create((
<CardActionIcon actionIcon="overflow" />
)).toJSON();
expect(tree).toMatchSnapshot();
});
});
75 changes: 75 additions & 0 deletions src/Card/tests/__snapshots__/CardActionIcon.test.jsx.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`<CardActionIcon /> renders with "overflow" option for actionIcon prop 1`] = `
<div
className="d-flex flex-wrap pgn__card-close-container"
>
<button
aria-label="See more"
className="btn-icon btn-icon-inverse-dark btn-icon-md mr-2"
onClick={[Function]}
type="button"
>
<span
className="btn-icon__icon-container"
>
<span
className="pgn__icon btn-icon__icon"
>
<svg
aria-hidden={true}
fill="none"
focusable={false}
height={24}
role="img"
viewBox="0 0 24 24"
width={24}
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M12 8c1.1 0 2-.9 2-2s-.9-2-2-2-2 .9-2 2 .9 2 2 2zm0 2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2zm0 6c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z"
fill="currentColor"
/>
</svg>
</span>
</span>
</button>
</div>
`;

exports[`<CardActionIcon /> renders without props 1`] = `
<div
className="d-flex flex-wrap pgn__card-close-container"
>
<button
aria-label="Close"
className="btn-icon btn-icon-inverse-dark btn-icon-md mr-2"
onClick={[Function]}
type="button"
>
<span
className="btn-icon__icon-container"
>
<span
className="pgn__icon btn-icon__icon"
>
<svg
aria-hidden={true}
fill="none"
focusable={false}
height={24}
role="img"
viewBox="0 0 24 24"
width={24}
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M19 6.41L17.59 5 12 10.59 6.41 5 5 6.41 10.59 12 5 17.59 6.41 19 12 13.41 17.59 19 19 17.59 13.41 12 19 6.41z"
fill="currentColor"
/>
</svg>
</span>
</span>
</button>
</div>
`;