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

[ControlGroup] Add border-radius inheritance support to popovers #2731

Merged

Conversation

badams
Copy link
Contributor

@badams badams commented Jul 26, 2018

Fixes #2730

Changes proposed in this pull request:

Adding border-radius: inherit to the popover-target and popover-wrapper will pass it on to whatever component is wrapped (eg. button)

Screenshot

Before:
screen shot 2018-07-27 at 10 55 05 am

After:
screen shot 2018-07-27 at 10 54 47 am

@giladgray
Copy link
Contributor

@badams can you please fix the CircleCI build? I cannot accept a PR without a build.

@badams
Copy link
Contributor Author

badams commented Jul 30, 2018

@giladgray sorted, forgot to setup CircleCi when I nuked my fork 😆

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

nice fix, but please put this code in the logically correct place.

.#{$ns}-popover-wrapper,
.#{$ns}-popover-target {
border-radius: inherit;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please move this next to the border-radius styles around line 188. this location is smack in the middle of a whole bunch of z-index logic.

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

thank you @badams!

@giladgray giladgray merged commit 5f5d5bc into palantir:develop Jul 30, 2018
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.

2 participants