Skip to content

Conversation

beefchimi
Copy link
Contributor

@beefchimi beefchimi commented Nov 13, 2018

A quick one two-liner.

I noticed that the border-radius for <Popover /> was funky, and tracked it down to inconsistent border-radius values.

This PR resolves the visual issue.

Before

Here you can see some of the Popover background poking through in the top left and right corner:

before

After

Top corners now resolved:

after

@ghost
Copy link

ghost commented Nov 13, 2018

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@danrosenthal
Copy link

@HYPD or @sarahill what is the intended border radius on this component? Should it be large or just the base?

@HYPD
Copy link
Contributor

HYPD commented Nov 13, 2018

@danrosenthal base would be the correct value to use here.

@sarahill @ry5n Should we document these values in the style guide and ui kit? If base and large are available in code, designers may be asked which one to use. We do this with spacing already here
https://polaris.shopify.com/design/spacing#section-spacing-in-code

@beefchimi beefchimi force-pushed the consistent-popover-radius branch from 4d54d33 to fab9a2d Compare November 13, 2018 17:37
@beefchimi
Copy link
Contributor Author

@HYPD updated to use the base variant

@danrosenthal danrosenthal added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Nov 13, 2018
@beefchimi beefchimi merged commit a0d6c3c into master Nov 13, 2018
@ghost
Copy link

ghost commented Nov 13, 2018

🎉 Thanks for your contribution to Polaris React!

@beefchimi beefchimi deleted the consistent-popover-radius branch November 13, 2018 18:34
@AndrewMusgrave AndrewMusgrave temporarily deployed to production November 14, 2018 20:23 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants