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 5 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
33 changes: 31 additions & 2 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,10 @@ 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.

offsetTop: 0,
stickyClass: '_sticky'
},

/**
Expand Down Expand Up @@ -41,8 +44,34 @@ define([

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good. Thank you


if (typeof this.options.spacingTop === 'function') {
Copy link

Choose a reason for hiding this comment

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

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.

offset += this.options.spacingTop();
} else {
offset += this.options.spacingTop;
}

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


var offsetTop = 0;
if (typeof this.options.offsetTop === 'function') {
offsetTop = this.options.offsetTop();
} else {
offsetTop = this.options.offsetTop;
}
Copy link

Choose a reason for hiding this comment

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

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');

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.


if (offset < this.options.offsetTop) {
Copy link

Choose a reason for hiding this comment

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

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.

offset = 0;
}
}

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

Expand Down