Skip to content
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
2 changes: 2 additions & 0 deletions UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f

### Bug fixes

- Removed extraneous space in `MediaCard` when card has no actions ([#4538](https://github.com/Shopify/polaris-react/pull/4538))

### Documentation

### Development workflow
Expand Down
17 changes: 9 additions & 8 deletions src/components/MediaCard/MediaCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,15 @@ export function MediaCard({
portrait && styles.portrait,
);

const actionMarkup = (
<div className={actionClassName}>
<ButtonGroup>
{primaryActionMarkup}
{secondaryActionMarkup}
</ButtonGroup>
</div>
);
const actionMarkup =
primaryActionMarkup || secondaryActionMarkup ? (
Copy link
Member

Choose a reason for hiding this comment

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

If we had codecov still active it would complain about not adding tests for this and we don't have visual regression testing at the moment so I think it worth adding a couple of tests here. I think we can just test to make sure button group doesn't render if primary or secondary actions are passed in

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! I added a simple test in f729d60

<div className={actionClassName}>
<ButtonGroup>
{primaryActionMarkup}
{secondaryActionMarkup}
</ButtonGroup>
</div>
) : null;

const mediaCardClassName = classNames(
styles.MediaCard,
Expand Down
20 changes: 20 additions & 0 deletions src/components/MediaCard/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,26 @@ Use when there are two distinct actions merchants can take on the information in
</MediaCard>
```

### Media card with no actions

Use when media card does not require any actions.

```jsx
<MediaCard
title="Getting Started"
description="Discover how Shopify can power up your entrepreneurial journey."
Comment on lines +220 to +221
Copy link
Contributor

Choose a reason for hiding this comment

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

This content wouldn't be something we'd want without a call to action, but that's something that can be addressed separately.

popoverActions={[{content: 'Dismiss', onAction: () => {}}]}
>
<img
alt=""
width="100%"
height="100%"
style={{objectFit: 'cover', objectPosition: 'center'}}
src="https://burst.shopifycdn.com/photos/business-woman-smiling-in-office.jpg?width=1850"
/>
</MediaCard>
```

### Video card

Use to provide a consistent layout for contextual learning content. Use to wrap thumbnails of educational videos about Shopify features in context.
Expand Down
36 changes: 36 additions & 0 deletions src/components/MediaCard/tests/MediaCard.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {Heading, Popover, Button, ActionList, Badge} from 'components';
import {mountWithApp} from 'test-utilities';

import {MediaCard} from '../MediaCard';
import styles from '../MediaCard.scss';

const mockProps = {
children: <img alt="" />,
Expand Down Expand Up @@ -48,6 +49,41 @@ describe('<MediaCard>', () => {
expect(videoCard).toContainReactComponent('p', {children: description});
});

it('does not render a wrapper around actions when primaryAction and secondaryAction are empty', () => {
const videoCard = mountWithApp(
<MediaCard
{...mockProps}
primaryAction={undefined}
secondaryAction={undefined}
/>,
);

expect(videoCard).not.toContainReactComponent('div', {
className: expect.stringContaining(styles.ActionContainer),
});
});

it('renders a wrapper around actions when primaryAction and secondaryAction are provided', () => {
const mockAction = {content: 'test'};
const videoCardWithPrimaryAction = mountWithApp(
<MediaCard {...mockProps} primaryAction={mockAction} />,
);
const videoCardWithSecondaryAction = mountWithApp(
<MediaCard
{...mockProps}
primaryAction={undefined}
secondaryAction={mockAction}
/>,
);

expect(videoCardWithPrimaryAction).toContainReactComponent('div', {
className: expect.stringContaining(styles.ActionContainer),
});
expect(videoCardWithSecondaryAction).toContainReactComponent('div', {
className: expect.stringContaining(styles.ActionContainer),
});
});

it('renders a Button with the primaryAction', () => {
const primaryAction = {content: 'test primary action'};
const videoCard = mountWithApp(
Expand Down