stevenbenner / jquery-powertip

:speech_balloon: A jQuery plugin that creates hover tooltips.
https://stevenbenner.github.io/jquery-powertip/
MIT License
821 stars 137 forks source link

Change the arrow of the tooltip to same color as the tooltip. #160

Closed arpan29 closed 6 years ago

arpan29 commented 7 years ago

The ability to add popupClass gives a great feature to add custom CSS to the tooltip. However, those custom changes do not apply to the arrow of the tooltip. This will help the arrow to inherit the border-color from the popupClass defined.

If the popupClass is not defined, it will take the default style.

stevenbenner commented 7 years ago

This is a good idea. I didn't think about arrow inheritance at all when support for custom classes was added.

Two requests:

  1. Check out my review comment. I think you may be able to get the same effect by changing the current border rules to 10px solid inherit.
  2. Can you make this same change to all of the CSS files?
arpan29 commented 7 years ago

Sure. Let me do that and update the Pull Request.

arpan29 commented 7 years ago

For point 1, I tried using the 10px solid inherit. But somehow, I am getting an error on Chrome as "invalid property value". Below is the screenshot for the same.

screen shot 2017-07-16 at 7 40 18 pm

As for point 2, the changes to all other css files have been done.

stevenbenner commented 7 years ago

Ahh, yep according to the MDN Shorthand properties article:

Only the individual properties values can inherit. As missing values are replaced by their initial value, it is impossible to allow inheritance of individual properties by omitting them. The keyword inherit can be applied to a property, but only as a whole, not as a keyword for one value or another. That means that the only way to make some specific value to be inherited is to use the longhand property with the keyword inherit.

So to use inherit we must use the longhand property rules:

border-top-width: 10px;
border-top-style: solid;
border-top-color: inherit;

This is probably the way to go, because right now the CSS looks like this:

border-top: 10px solid rgba(0, 0, 0, 0.8);
border-top-color: inherit;

So the rgba() is immediately overridden in the same CSS file, thus doesn't actually do anything.

I'm thinking that it makes sense to split the existing border-xxx rules into border-xxx-width and border-xxx-style. What do you think?

arpan29 commented 7 years ago

Yes, it seems like a good idea to define each border property explicitly.

Also, I feel that the border-top-style: solid, border-top-width: 10px, border-top-color: inherit is repeated in the stylesheets at a lot of places. Instead of that, we could have a common class powertip-border-top and apply it directly to the DOM elements wherever required. What do you think about this change?

stevenbenner commented 7 years ago

Do you mean an additional class for the repeated arrow rules? So the DOM for an open tooltip would be something like this?

<div id="powerTip" class="n powertip-border-top">
    This is a tooltip with "north" placement.
</div>

I'd rather have as few CSS identifiers as possible, even if it requires some repeated CSS rules. My concern is that the JavaScript code has to manage these classes during operation, so there would be new code needed to associate the placement values to a specific arrow class, and the cleanup code would have to be updated to remove all arrow classes when the tooltip is closed.

Or were you thinking of something else? Can you give me an example?

stevenbenner commented 6 years ago

Since you haven't responded for a while, I'll do the extra clean up on my end.

Pull request accepted! Merged in commit da7ffbfffa649dbc6f59ef7f913951e9db12e112.

I have squashed your commits together into one commit and added a couple fixes. I removed a trailing tab on one of the lines, added the hex color code fallback for the default CSS file (this is for old-IE support), and I removed the border-color properties from the other CSS files (it appears that they were unnecessary because the color was already defined in the border shorthand property). I'll add the other tweaks later.

Thank you very much for the patch!