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.

Sign up for GitHub

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

Stickyjs improvements #7139

Merged
merged 18 commits into from
Jun 27, 2017
Merged

Stickyjs improvements #7139

Changes from 15 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8bfa048
SpacingTop feature added
vovayatsyuk Oct 22, 2016
996d213
Trailing spaces was removed
vovayatsyuk Oct 22, 2016
a7254cf
Added stickyClass (css class) feature.
vovayatsyuk Oct 22, 2016
798469a
Added offsetTop feature
vovayatsyuk Oct 22, 2016
9f926ef
Merge branch 'develop' into improvements-stickyjs
vovayatsyuk Mar 17, 2017
c6d309b
Merge branch 'develop' into improvements-stickyjs
vovayatsyuk Apr 4, 2017
766292c
Fixed offsetTop comparing bug
vovayatsyuk Apr 4, 2017
0e479a3
Added stuck variable for better 'if' statement readability
vovayatsyuk Apr 4, 2017
ae7f764
Duplicated option value detection logic moved to separate function
vovayatsyuk Apr 5, 2017
dde585c
Shorthand method definition syntax replaced with standard one
vovayatsyuk Jun 14, 2017
5071e54
OptionToNumber method refactored:
vovayatsyuk Jun 14, 2017
e6795cb
Better code readability
vovayatsyuk Jun 14, 2017
7ae6be0
Fixed JSHint notices
vovayatsyuk Jun 14, 2017
db648a6
ESLint and JSCS fixes
vovayatsyuk Jun 14, 2017
fc0ae57
offsetTop option renamed into stickAfter as it has nothing with eleme…
vovayatsyuk Jun 19, 2017
febbd88
DocBlocks added for the new options
vovayatsyuk Jun 23, 2017
d715b42
DocBlock added for the 'container' option
vovayatsyuk Jun 23, 2017
62b822c
Typo error in DocBlock fixed
vovayatsyuk Jun 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions lib/web/mage/sticky.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,26 @@ define([

$.widget('mage.sticky', {
options: {
container: ''
container: '',
spacingTop: 0,
Copy link
Contributor

@omiroshnichenko omiroshnichenko Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still thinking about a new name for it 😫

Copy link
Member Author

@vovayatsyuk vovayatsyuk Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

stickAfter: 0,
stickyClass: '_sticky'
},

/**
* Retrieve option value
* @param {String} option
* @return {*}
* @private
*/
_getOptionValue: function (option) {
var value = this.options[option] || 0;

if (typeof value === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

value = this.options[option]();
}

return value;
},

/**
Expand All @@ -35,14 +54,29 @@ define([
*/
_stick: function () {
var offset,
isStatic;
isStatic,
stuck,
stickAfter;

isStatic = this.element.css('position') === 'static';

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

offset = Math.max(0, Math.min(offset, this.maxOffset));
this.element.css('top', offset);

stuck = this.element.hasClass(this.options.stickyClass);
stickAfter = this._getOptionValue('stickAfter');

if (offset && !stuck && offset < stickAfter) {
offset = 0;
}

this.element
.toggleClass(this.options.stickyClass, offset > 0)
.css('top', offset);
}
},

Expand Down