Skip to content

Conversation

@emilycritter
Copy link
Contributor

@emilycritter emilycritter commented Oct 21, 2021

WHY are these changes introduced?

Fixes #4534

In vertical view (smaller viewports), MediaCard without primaryAction or secondaryAction has extra space at the bottom.

WHAT is this pull request doing?

This PR adds a conditional to check for actions thus removing an extra, empty div that was resulting in extraneous space at the bottom of the card.

Before:
image showing the card component with extra white space at the bottom

After:
image showing the absence of white space when no action buttons are included

@ghost
Copy link

ghost commented Oct 21, 2021

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@ghost ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Oct 21, 2021
@emilycritter
Copy link
Contributor Author

Is this project participating in hacktoberfest?

@kaelig
Copy link
Contributor

kaelig commented Oct 21, 2021

Is this project participating in hacktoberfest?

Not that I know, but a huge thank you for this contribution!

Could you also add a new example in https://github.com/emilycritter/polaris-react/blob/fix/media-card-extraneous-space/src/components/MediaCard/README.md to illustrate this use-case?

@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Oct 28, 2021
Comment on lines +220 to +221
title="Getting Started"
description="Discover how Shopify can power up your entrepreneurial journey."
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.

@kaelig
Copy link
Contributor

kaelig commented Oct 28, 2021

Not sure adding a test is useful here, since visual regression tests would catch any issues with how layout is impacted by future changes.

I'll let the maintainers decide!

@kaelig kaelig requested a review from a team October 28, 2021 22:27
</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

@kaelig
Copy link
Contributor

kaelig commented Nov 3, 2021

Since some of the tests couldn't complete (probably due to the fact the PR comes from a fork), an admin will have to merge this PR.

@kyledurand kyledurand merged commit 1a228a2 into Shopify:main Nov 3, 2021
@ghost
Copy link

ghost commented Nov 3, 2021

🎉 Thanks for your contribution to Polaris React!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MediaCard] Extraneous space when card has no actions

3 participants