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.

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

Added spacingTop feature to sticky plugin. #6536

Closed

Conversation

vovayatsyuk
Copy link
Member

@vovayatsyuk vovayatsyuk commented Sep 8, 2016

By default sticky element get sticked at the top of the screen
without any gap. This commit allows to add it in options object:

$('#element').sticky({
    container: '#container',
    spacingTop: 25
});

By default sticky element get sticked at the top of the screen
without any gap. This commit allows to add it in options object:

```js
$('#element').sticky({
    container: '#container',
    offsetTop: 25
});
```
@ihor-sviziev
Copy link
Contributor

@vovayatsyuk offset could be different for desktop & mobile, so would be great to calculate it dynamically.

@vovayatsyuk
Copy link
Member Author

I can extend this functionality to allow to pass a function as a offsetTop parameter.
@ihor-sviziev what do you think?

@vovayatsyuk
Copy link
Member Author

vovayatsyuk commented Sep 13, 2016

Hi, I just realized that it's better to call this parameter as a spacingTop or similar.

offsetTop could be used for another feature - make element sticky when it scroll out from the screen for X pixels or whatever.

What do you think?

@vovayatsyuk vovayatsyuk changed the title Added offsetTop feature to sticky plugin. Added spacingTop feature to sticky plugin. Sep 20, 2016
@que-etc
Copy link
Contributor

que-etc commented Oct 18, 2016

It'd be better to tend towards the more generic solution, which is to define the way an element is being displayed in sticky mode with a CSS declaration.

Unfortunately, current implementation doesn't provide any specific class when an element becomes fixed. Could you please add one? E.g.:

options: {
    stickyClass: '_sticky'
},

_stick: function () {
    //...

    this.element.togglClass(this.options.stickyClass, isElementFixed);
}

@vovayatsyuk
Copy link
Member Author

vovayatsyuk commented Oct 18, 2016

Yes, I thought about this too, and wanted to create another pull request with css solution.

In order to keep current implementation, I though it could be defined optionally:

$('#element').sticky({
    container: '#container',
    stickyClass: '_sticky' // empty by default
});

Implementation example:

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

p.s. However, the most wise solution is to use some popular third-party library instead of own sticky script. Don't you think so?

@que-etc
Copy link
Contributor

que-etc commented Oct 18, 2016

Oh, I see what you wanna do here. Well, I meant to add this flag merely as a state indicator and to retain current positioning via inline styles. As you've mentioned, this way we can avoid breaking changes.

p.s. However, the most wise solution is to use some popular third-party library instead of own sticky script.

I hear you. To be frank, I'd rather replace it with a polyfill of position: sticky CSS rule.

@vovayatsyuk
Copy link
Member Author

Oh, I see. Yes, state indicator could resolve PR subject and gives a lot more.

I think we can close this PR in favor of another one with class name indicator.

@vasiliyseleznev
Copy link

@vovayatsyuk thanks for you contribution. Looking forward to get new PR created.

@vovayatsyuk
Copy link
Member Author

vovayatsyuk commented Oct 18, 2016

Sorry, my mistake.

Class name indicator is a great thing and should be added too, but it doesn't give me an
ability to make sticky element with specified gap smoothly.

My use case is:

  1. Element isn't sticked and it does not have margin and paddings (I can't use them in my case)
  2. Element must stick without touching top edge of the screen. (I've used spacingTop parameter for this)

Can you reopen this PR?

magento-engcom-team pushed a commit that referenced this pull request Jan 21, 2021
[Arrows] MC-23915: Revert 23915 because it caused static tests failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants