Stickyjs improvements #7139

vovayatsyuk · 2016-10-22T13:14:56Z

Added _sticky class for "stuck" element
Added spacingTop feature (Added spacingTop feature to sticky plugin. #6536)
Added stickAfter feature, that allows postponing sticking, until element will go out from the screen for stickAfter pixels.

Screenshots

Default Settings

With spacingTop = 25

With spacingTop = 25 and stickAfter = 60

vasiliyseleznev · 2016-10-25T10:36:55Z

Hi @vovayatsyuk,

Thanks for second loop with this improvement. We will go ahead and accept it.
Internal ticket: MAGETWO-60046

okorshenko · 2017-03-17T03:21:05Z

@vovayatsyuk could you please resolve conflicts so that we can proceed with PR acceptance?

vovayatsyuk · 2017-03-17T08:44:30Z

Done.

okorshenko · 2017-03-17T14:25:38Z

@vovayatsyuk thank you for resolving the merge conflicts

okorshenko · 2017-03-27T12:06:40Z

lib/web/mage/sticky.js

@@ -41,8 +44,34 @@ define([

            if (!isStatic && this.element.is(':visible')) {
                offset = $(document).scrollTop() - this.parentOffset;


why do we need this line of code? Variable offset will be redefined in any case in line 49 or 51.
Could you please review this logic?

Maybe I'm missing the point, but it's not redefined at those lines, it just modified with the possible spacingTop parameter.

Sorry, can you comment on this? I don't understand how you'd like to modify the source.

looks good. Thank you

zetlen

There are some readability issues and at least one bug, but mostly this is great!

zetlen · 2017-04-03T18:15:50Z

lib/web/mage/sticky.js

@@ -41,8 +44,34 @@ define([

            if (!isStatic && this.element.is(':visible')) {
                offset = $(document).scrollTop() - this.parentOffset;
+
+                if (typeof this.options.spacingTop === 'function') {


You should check to see whether spacingTop is a Number, or whether it returns a Number.

var spacingTop = this.options.spacingTop || 0; if (typeof this.options.spacingTop === 'function') { spacingTop = this.options.spacingTop(); } spacingTop = Number(spacingTop); if (isNaN(spacingTop)) { throw new Error('sticky: spacingTop must be a Number'); } offset += this.options.spacingTop;

See below for more notes.

zetlen · 2017-04-03T18:26:18Z

lib/web/mage/sticky.js

+                        offsetTop = this.options.offsetTop();
+                    } else {
+                        offsetTop = this.options.offsetTop;
+                    }


This pattern happens twice, and there may be further configurable numbers, so it probably should be separated into a private function.
At the top of this file:

function optionToNumber(self, option) { var value = self.options[option] || 0; if (typeof value === 'function') { value = self.options[option](); } var converted = Number(value); if (isNaN(converted)) { throw Error('sticky: Could not convert supplied option "' + option + '" to Number from "' + value + '"'); } return converted; }

And then here, you can just have:

var offsetTop = optionToNumber(this, 'offsetTop'); if (offset < offsetTop) { offset = 0; }

And above, you can just have:

offset += optionToNumber(this, 'spacingTop');

Nice optimization, but is it ok to send this? Maybe it's better to place the function inside the widget to get rid of this parameter? I know the function is not needed there, but I'm not sure what is better: "send this" or create a function inside the widget?

It's okay to send this from an object method to a locally declared function. It's a way to do real privacy in JavaScript. I see it in lots of codebases! But the way that you implemented it, as an underscore-prefixed method, is also just fine.

zetlen · 2017-04-03T18:27:18Z

lib/web/mage/sticky.js

+                        offsetTop = this.options.offsetTop;
+                    }
+
+                    if (offset < this.options.offsetTop) {


This line is a bug; this.options.offsetTop may still be a function. You might have means if (offset < offsetTop). The refactor I suggest in my above comments should fix it, though.

zetlen · 2017-04-03T18:28:41Z

lib/web/mage/sticky.js

+
+                if (offset &&
+                    this.options.offsetTop &&
+                    !this.element.hasClass(this.options.stickyClass)) {


Can you explain the desired logic here? It's hard to tell from the code what !this.element.hasClass(this.options.stickyClass) means for the business logic. Does it mean "the element is currently displayed as sticky" or "the element is NOT currently displayed as sticky"? For more readability, you could assign it to a variable stuck.

The logic inside of this if should work for the non-sticked element, when offsetTop is used. (It makes smooth return of the items with offsetTop parameter, when scrolling up)

In other words, I check if an element should be stuck (offset > 0), offsetTop is used and an element is not yet stuck.

vherasymenko · 2017-05-15T14:47:24Z

@vovayatsyuk Hi. I can`t apply your improvement.
I did checkout your branch. And I do not have any changes in my application. Behavior is still old.

vovayatsyuk · 2017-05-16T13:11:39Z

You should redeploy static content:

Quick solution:

Remove old file:

$ bin/magento deploy:mode:set developer
$ find pub/static -regex ".*/sticky.js" -exec rm -rf {} \;;

Refresh the page in browser

vherasymenko · 2017-05-16T13:50:28Z

I do it same.

I try install magento from your branch https://github.com/vovayatsyuk/magento2/tree/improvements-stickyjs
Make composer install
bin/magento deploy:mode:set developer
bin/magento static-content:deploy
but in pub/static (sticky.js is missing)

I Also try apply your commit like a patch for my project and nothing changing.

vovayatsyuk · 2017-05-16T14:03:21Z

Sorry, it's a bit off topic to talk about magento deployment here.

If you'd like to see how the patch is working, you can open deployed file and apply the changes manually.

If you use English locale and luma theme, you'll find the file right here: pub/static/frontend/Magento/luma/en_US/mage/sticky.js

If that will not work for you, I really don't know how to help you, sorry.

p.s. did you flush browser cache?
p.p.s What changes do you want to see? My changes do not affect any visible parts of magento. It's up to third-party developers to use the extended functionality.

omiroshnichenko · 2017-06-06T07:58:47Z

lib/web/mage/sticky.js

+            stickyClass: '_sticky'
+        },
+
+        _optionToNumber(option) {


This is not correct function declaration. Move it out of widget in to module private scope or declare this like method of widget. Also, i have a question, why are you check on type of function offsetTop and spacingTop when it's declared below as Number?

Hey, thank you @omiroshnichenko, I missed that. ES6 object shorthand is not currently allowed in Magento JavaScript. This should change to:

_optionToNumber: function(option) {

omiroshnichenko

Please, validate it with static test locallygrunt static --file=lib/web/mage/sticky.js or rerun travis build.

omiroshnichenko · 2017-06-06T08:05:35Z

lib/web/mage/sticky.js

+
+            var converted = Number(value);
+            if (isNaN(converted)) {
+                throw Error(


We don't have practice to throwing errors in magento js.

omiroshnichenko · 2017-06-06T08:21:49Z

lib/web/mage/sticky.js

@@ -11,7 +11,26 @@ define([

    $.widget('mage.sticky', {
        options: {
-            container: ''
+            container: '',
+            spacingTop: 0,


You have been provide new settings properties that by default is Number, no reason to check it on another types. User can't change this property, only developer, when developer will be configuring sticky widget he can look at type of parameter. Also, offsetTop it's native property of HTMLElement would be nice to change it to another, e.g. blockOffsetTop.

Still thinking about a new name for it 😫

What would you say to rename it to stickAfter or triggerOffset or stickOffset?

I've picked a stickAfter. It seems that it better and self-explanatory name.

omiroshnichenko · 2017-06-06T08:27:06Z

lib/web/mage/sticky.js

+                var stuck = this.element.hasClass(this.options.stickyClass);
+                if (offset && this.options.offsetTop && !stuck) {
+                    var offsetTop = this._optionToNumber('offsetTop');
+                    if (offset < offsetTop) {


This check can be include in upper if statement via AND(&&) operator.

okorshenko · 2017-06-13T22:28:55Z

Hi @vovayatsyuk
Could you please check review comments?

- Removed option type validation - Renamed into getOptionValue as it can be used for any option types

…nt's offsetTop property

vovayatsyuk · 2017-06-19T08:43:56Z

Sorry for the delay. I think I'm done.

omiroshnichenko · 2017-06-23T07:16:16Z

lib/web/mage/sticky.js

+        _getOptionValue: function (option) {
+            var value = this.options[option] || 0;
+
+            if (typeof value === 'function') {


Why you check value on function type? In what case it can be function?

It could be useful to set a function for stickAfter parameter that will calculate sticky element height when you wish to stick element after it goes out of the viewport and the element could change its height dynamically (shopping cart for example)
You can use different spacing top value for different screen size/orientation also.

Ok, but can you leave docblock with description. Thanks.

vovayatsyuk added 4 commits October 22, 2016 13:41

SpacingTop feature added

8bfa048

Trailing spaces was removed

996d213

Added stickyClass (css class) feature.

a7254cf

Added offsetTop feature

798469a

vasiliyseleznev assigned que-etc Oct 24, 2016

vasiliyseleznev added the Area: Frontend label Oct 24, 2016

vasiliyseleznev added improvement Progress: accept labels Oct 25, 2016

MomotenkoNatalia added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Oct 26, 2016

okorshenko added the Progress: needs update label Mar 17, 2017

Merge branch 'develop' into improvements-stickyjs

9f926ef

okorshenko removed the Progress: needs update label Mar 17, 2017

okorshenko self-assigned this Mar 17, 2017

okorshenko added this to the March 2017 milestone Mar 17, 2017

okorshenko suggested changes Mar 27, 2017

View reviewed changes

okorshenko added Progress: needs update and removed Progress: accept labels Mar 27, 2017

okorshenko modified the milestones: March 2017, April 2017 Apr 2, 2017

okorshenko removed the Progress: needs update label Apr 2, 2017

zetlen suggested changes Apr 3, 2017

View reviewed changes

vovayatsyuk added 3 commits April 4, 2017 10:18

Merge branch 'develop' into improvements-stickyjs

c6d309b

Fixed offsetTop comparing bug

766292c

Added stuck variable for better 'if' statement readability

0e479a3

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

omiroshnichenko reviewed Jun 6, 2017

View reviewed changes

okorshenko added Progress: needs update and removed Progress: accept labels Jun 6, 2017

vovayatsyuk added 6 commits June 14, 2017 10:32

Shorthand method definition syntax replaced with standard one

dde585c

OptionToNumber method refactored:

5071e54

- Removed option type validation - Renamed into getOptionValue as it can be used for any option types

Better code readability

e6795cb

Fixed JSHint notices

7ae6be0

ESLint and JSCS fixes

db648a6

offsetTop option renamed into stickAfter as it has nothing with eleme…

fc0ae57

…nt's offsetTop property

omiroshnichenko suggested changes Jun 23, 2017

View reviewed changes

vovayatsyuk added 3 commits June 23, 2017 20:53

DocBlocks added for the new options

febbd88

DocBlock added for the 'container' option

d715b42

Typo error in DocBlock fixed

62b822c

omiroshnichenko approved these changes Jun 24, 2017

View reviewed changes

okorshenko removed the Progress: needs update label Jun 26, 2017

magento-team merged commit 62b822c into magento:develop Jun 27, 2017

This pull request was closed.

		@@ -41,8 +44,34 @@ define([

		if (!isStatic && this.element.is(':visible')) {
		offset = $(document).scrollTop() - this.parentOffset;

Stickyjs improvements #7139

Stickyjs improvements #7139

Conversation

vovayatsyuk commented Oct 22, 2016 • edited by okorshenko Loading

vasiliyseleznev commented Oct 25, 2016 • edited Loading

okorshenko commented Mar 17, 2017

vovayatsyuk commented Mar 17, 2017

okorshenko commented Mar 17, 2017

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

zetlen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vherasymenko commented May 15, 2017

vovayatsyuk commented May 16, 2017

vherasymenko commented May 16, 2017

vovayatsyuk commented May 16, 2017

omiroshnichenko Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omiroshnichenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omiroshnichenko Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vovayatsyuk Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okorshenko commented Jun 13, 2017

vovayatsyuk commented Jun 19, 2017

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

vovayatsyuk commented Oct 22, 2016 •

edited by okorshenko

Loading

vasiliyseleznev commented Oct 25, 2016 •

edited

Loading

omiroshnichenko Jun 6, 2017 •

edited

Loading

omiroshnichenko Jun 6, 2017 •

edited

Loading

vovayatsyuk Jun 15, 2017 •

edited

Loading