Major skin refactor: new features #41

Tofandel · 2019-03-20T01:12:33Z

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

css/protip.scss

wintercounter · 2019-03-20T08:21:46Z

src/Plugin.js

+		 * @param e
+		 * @return {protipUpdate}
+		 */
+		protipUpdate: function (e) {


You just need to call protipShow() on the item to manually update it which makes this function obsolete.

That's not exactly what this does, maybe I should add the logic of update to show then

Basically the problem I had is that the title is set to an html element with a selector, but when the html element is updated, the content of the protip is not..

This function updates the content of the protip if it was modified

I did check the logic. But with protipShow it'll also reload the content.

wintercounter

While I support the ability to set color, bgColor and border through props, we also need to be backwards compatible because I don't want to introduce braking changes. CSS skinning is also not only about adjusting those 3 properties but any other properties of a tooltip. This means that the mixin method has to stay.
Also the generated CSS file is just the default one. You don't need to use it, that's why there is a mixin, you can plug it in into your existing system generating only the skin you want...

Please revert the code style changes both in JS and SCSS. I know there is no eslint config specified, that can be a separate discussion. If you want to add eslint+prettier then I have a preferred approach for that, we can talk about it. All in all style changes should not be in the scope if this PR.

See my comments for several lines also.

Tofandel · 2019-03-20T10:37:58Z

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 · 2019-03-20T11:41:19Z

src/Class.js

@@ -354,16 +359,16 @@
 			var el                = $(ev.target);
 			var container         = el.closest('.' + C.SELECTOR_PREFIX + C.SELECTOR_CONTAINER) || false;
 			var source            = el.closest(C.DEFAULT_SELECTOR);
-			var sourceInstance    = this._isInited(source) ? this.getItemInstance(source) : false;
+			//var sourceInstance    = this._isInited(source) ? this.getItemInstance(source) : false;


Remove if not needed.

wintercounter · 2019-03-20T11:45:54Z

src/Item.js

@@ -228,6 +297,8 @@
 				style = new PositionCalculator(this);
 			}

+			this.applyColors();


Please just merge with the style object. Now your applying CSS styles twice (// Apply styles, classes)

wintercounter · 2019-03-20T11:47:37Z

src/Item.js

@@ -253,6 +325,7 @@
 		 * @param position
 		 */
 		applyPosition: function(position){
+			this.data.position = position;


Position should have been already set in data. This method is not meant to be set that, only the attr of the element.

The position data is not changed with gravity, I'll switch it to the show method after GravityTester creation

wintercounter · 2019-03-20T11:55:22Z

README.md

@@ -142,6 +142,9 @@ Protip related attributes will always get a pt namespace so Protip won't conflic
 | **auto-hide**   | false     | *Bool, Number* | Tooltip will hide itself automatically after this timeout (milliseconds).                                                                                                                                    |
 | **auto-show**   | false     | *Bool*         | Automatically show tooltip for this element on load (stickies will be shown anyway).                                                                                                                                    |
 | **mixin**       | undefined | *String*       | Tooltip mixins to use. Separated by spaces.                                                                                                                                    |
+| **background**  | undefined | *String*       | You can specify the tooltip background with this option. (Eg: "#333", "rgba(20,20,20,.8)")                                                                                                                                    |


New props should be added to demo also.

What demo? Do you mean the test folder or http://protip.rocks/ ?

Yes, the website.

wintercounter · 2019-03-20T12:01:08Z

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 · 2019-03-20T12:03:58Z

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 · 2019-03-20T12:50:40Z

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 · 2019-03-20T13:09:07Z

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:

Created <div id="c">Content1</div>
Called $('.any-element').protipShow({ title: '#c' }) => Tooltip appeared
Changed Content1 to Content2
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 · 2019-03-20T13:12:21Z

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

Tofandel · 2019-03-20T13:17:45Z

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 · 2019-03-20T13:47:04Z

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 · 2019-03-20T13:52:14Z

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

Tofandel · 2019-03-20T14:27:22Z

Tested and fixed everything, have a check at the test

wintercounter · 2019-03-28T12:55:29Z

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

Tofandel added 2 commits March 20, 2019 01:45

Major skin refactor - colors and borders are now handled through js

0565bf4

Major skin refactor - colors and borders are now handled through js

39b481a

wintercounter reviewed Mar 20, 2019

View reviewed changes

css/protip.scss Outdated Show resolved Hide resolved

wintercounter reviewed Mar 20, 2019

View reviewed changes

css/protip.scss Outdated Show resolved Hide resolved

wintercounter reviewed Mar 20, 2019

View reviewed changes

wintercounter requested changes Mar 20, 2019

View reviewed changes

Tofandel added 2 commits March 20, 2019 11:56

Maximize Backward compatibility and restore previous formatting

76b928c

Maximize Backward compatibility and restore previous formatting

98bcf72

wintercounter self-assigned this Mar 20, 2019

wintercounter reviewed Mar 20, 2019

View reviewed changes

Restore the mixin but don't actually use it

a7c2202

Fix protip showing only once

3a2b9d0

Fix wrong corner values in doc and tests

fbdf2e6

Fix item instance deletion

26193bf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major skin refactor: new features #41

Major skin refactor: new features #41

Tofandel commented Mar 20, 2019

wintercounter Mar 20, 2019

Tofandel Mar 20, 2019

wintercounter Mar 20, 2019

wintercounter left a comment

Tofandel commented Mar 20, 2019 •

edited

Loading

wintercounter Mar 20, 2019

wintercounter Mar 20, 2019

wintercounter Mar 20, 2019

Tofandel Mar 20, 2019

wintercounter Mar 20, 2019

Tofandel Mar 20, 2019

wintercounter Mar 20, 2019

wintercounter commented Mar 20, 2019

wintercounter commented Mar 20, 2019 •

edited

Loading

Tofandel commented Mar 20, 2019 •

edited

Loading

wintercounter commented Mar 20, 2019

Tofandel commented Mar 20, 2019

Tofandel commented Mar 20, 2019 •

edited

Loading

Tofandel commented Mar 20, 2019

Tofandel commented Mar 20, 2019 •

edited

Loading

Tofandel commented Mar 20, 2019

wintercounter commented Mar 28, 2019

Major skin refactor: new features #41

Are you sure you want to change the base?

Major skin refactor: new features #41

Conversation

Tofandel commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wintercounter left a comment

Choose a reason for hiding this comment

Tofandel commented Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wintercounter commented Mar 20, 2019

wintercounter commented Mar 20, 2019 • edited Loading

Tofandel commented Mar 20, 2019 • edited Loading

wintercounter commented Mar 20, 2019

Tofandel commented Mar 20, 2019

Tofandel commented Mar 20, 2019 • edited Loading

Tofandel commented Mar 20, 2019

Tofandel commented Mar 20, 2019 • edited Loading

Tofandel commented Mar 20, 2019

wintercounter commented Mar 28, 2019

Tofandel commented Mar 20, 2019 •

edited

Loading

wintercounter commented Mar 20, 2019 •

edited

Loading

Tofandel commented Mar 20, 2019 •

edited

Loading

Tofandel commented Mar 20, 2019 •

edited

Loading

Tofandel commented Mar 20, 2019 •

edited

Loading