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

Fix arrow colour on popover bottom #13164

Closed
wants to merge 2 commits into from
Closed

Fix arrow colour on popover bottom #13164

wants to merge 2 commits into from

Conversation

mattbearman
Copy link

Before:
before

After:
after

Stop arrow from being white on bottom popover, as the arrow sits on the title bar, so should match the title bar's background colour.

Stop arrow from being white on bottom popover, as the arrow sits on the title bar, so should match the title bar's background colour.
@cvrebert
Copy link
Collaborator

Could you post a live example (e.g. JS Fiddle) that demonstrates the problem?

@cvrebert cvrebert added the css label Mar 24, 2014
@cvrebert cvrebert added this to the v3.2.0 milestone Mar 24, 2014
@@ -108,9 +108,10 @@
&:after {
content: " ";
top: 1px;
margin-left: -@popover-arrow-width;
margin-left: -@popover-arrow-width - 1px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

LESS requires parentheses around arithmetic expressions; this isn't compiling correctly currently.

@cvrebert
Copy link
Collaborator

Please include /dist/css/bootstrap.css in your pull request, so that we can see+check the generated CSS too.

@mattbearman
Copy link
Author

Ah, didn't realise LESS is meant to have parentheses, it compiles fine on mine (less and jekyll-less ruby gems) I'll update that and include the generated CSS.

JSFiddles:
Current master - http://jsfiddle.net/5UAXa/1/
Fixed with my PR - http://jsfiddle.net/s8jqD/

Add parenthesis to LESS computed values
@cvrebert
Copy link
Collaborator

Yeah, please use our Grunt task when building/testing for a pull request, to ensure that you're using the correct versions of everything.

@mdo mdo removed this from the v3.2.0 milestone Mar 25, 2014
@mdo
Copy link
Member

mdo commented Mar 25, 2014

It's meant to be white 😁.

@mdo mdo closed this Mar 25, 2014
@mattbearman
Copy link
Author

Are honestly saying that when the popover is using the bottom positioning the arrow looks better white than grey?

I understand that when the popover is left, top or right the arrow should be white, because it matches the white of the popover body. But when bottom positioned, the arrow sits above the title bar, which is grey, so the white arrow doesn't look as good.

I've just noticed - look at this conversation, the arrow that points to my face is grey, to match the title bar, bootstrap's popovers would look better if they did the same.

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 25, 2014

Well, but not every popover is required to have a title bar, so it then would look bad if the arrow was gray and no title bar was present.

@mattbearman
Copy link
Author

Hmm, now that is a good point, I suppose you could have an additional class to make the popover's arrow grey for when it's positioned bottom with a title?

@hnrch02
Copy link
Collaborator

hnrch02 commented Mar 25, 2014

I guess we could kinda override the white triangle with a gray one, like this, but it seems to be a rather hacky solution...

@mdo
Copy link
Member

mdo commented Mar 25, 2014

I've just noticed - look at this conversation, the arrow that points to my face is grey, to match the title bar, bootstrap's popovers would look better if they did the same.

Yup, I designed both :). The Bootstrap popover is meant to tie into the inner border on popovers. Look closely and you'll see a 1px white inner border (set with padding). Subtle, but the colors make sense given that, and the optional header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants