wintercounter / Protip

A new generation jQuery Tooltip plugin
http://protip.rocks
MIT License
359 stars 38 forks source link

Major skin refactor: new features #41

Open Tofandel opened 5 years ago

Tofandel commented 5 years ago

Colors are now handled through js instead of css This gives way more flexibility for users that can edit the protip skin with 3 new properties background, color and border (The color schemes are still available but under the class constants and they simply change the default of those 3 properties depending the selected scheme)

I also refactored the square skin in just 1 property to reduce the css size, since it's the same as the default just with no border-radius

Borders are now supported: it's possible to add a 1px border solid with variable color to the protip

It was tricky because of the arrows (Using a semi transparent background with an arrow and a border will reveal the trick under the arrow and not be so pretty, for all the rest it works very nice)

The final css is around 70% lighter for just a 5% increase in js size

Tofandel commented 5 years ago

Should I readd the previous mixin in a new file? Since it's not really usefull anymore in the default skin

There is no real doc about the mixins and the mixin property of protip or how to generate your own skin, so at least it adds customization for users who don't want to dig to deep. In the end it's just a tooltip, so having to put scripts in place just to generate a skin hmm not so good

For the formatting, well I tried different formatting options but the code seems like it wasn't autoformatted I matched my paramaters as close as possible with the code style but for example

        applyPosition: function(position){
            this.el.protip.attr('data-' + C.DEFAULT_NAMESPACE + '-' + C.PROP_POSITION, position);
        },

        hide: function(force, preventTrigger) {

As you can see, in one function declaration there is a space before the bracket, not on the other one, someplaces, spaces, some places tabs, so whatever I set there were always differences.. And working with auto refactor is well.. let's just say there is less to worry about, I've reverted the js formatting, but an autoformatting would be nice

wintercounter commented 5 years ago

Yes, formatting can be handle, I agree on that, but not in the scope of this PR.

For backward compability yes. Everything should stay at the same place. The ability to add skin through class+CSS has to stay.

mixins is prop being documented even if it's just a bit. Nothing else just classes + CSS, but the docs about it could be better, I agree.

wintercounter commented 5 years ago

I just check the code now if it works in practice. Tooltips are only appearing once, never ever again. (used noDebug.html)

Update: Sticky tooltip are not working at all.

Tofandel commented 5 years ago

Yes, formatting can be handle, I agree on that, but not in the scope of this PR.

For backward compability yes. Everything should stay at the same place. The ability to add skin through class+CSS has to stay.

mixins is prop being documented even if it's just a bit. Nothing else just classes + CSS, but the docs about it could be better, I agree.

Skins can still be added through css, I've made more BC, if the skin and schemes of the protip are not in the predefined skins and scheme, no additional css is applied (unless the 3 properties are set)

So if style is added to a skin it stills works, if they modified the default skin colors hower without the !important keyword the selector will not override the inline style applied but sadly that BC cannot be fixed

I'll have a check as to why it's only appearing once

As for the content, are you sure it was updated (can you show me where?) because I remember trying everything and without explicitly recreating the protip the content wasn't refreshed, maybe a callback should be added to the mutation observer to track the source of the title changes and automatically updated without users having to call protipShow

wintercounter commented 5 years ago

MutationObserver is already causing performance issues in certain cases, I don't really want to add more.

protipShow is recreating the instance yes, that's why it's working. That mechanic behind it is there on purpose because it has to go though rerender on any property change.

I've did it from devTools on protip.rocks:

  1. Created <div id="c">Content1</div>
  2. Called $('.any-element').protipShow({ title: '#c' }) => Tooltip appeared
  3. Changed Content1 to Content2
  4. Called 2nd step again.

Internally protip will call show as show(force = true) which make it to skip certain step which are only needed for regular initialization.

Tofandel commented 5 years ago

Okay I removed, the update method, that was causing the only appear once issue

Tofandel commented 5 years ago

The data gravity = 2 is not working on the test, any idea what might cause this, was it working before?

Edit: okay some positions are not working as well, the position is not calculated, I'll investigate

Tofandel commented 5 years ago

Okay all corners position are not working but it seems unrelated to my changes The position calculated is this "corner-left-bottom" {left: "0px", top: "0px"} Is the calculation wrong ?

Tofandel commented 5 years ago

Okay found it the position of the tests are wrong they differ from the constants

Tofandel commented 5 years ago

Tested and fixed everything, have a check at the test

wintercounter commented 5 years ago

Just a heads up. Sorry, but I didn't have time so far to check. I'll do it the upcoming days.