Skip to content

Conversation

@lilpug
Copy link

@lilpug lilpug commented Dec 10, 2016

This addition adds the extra setting option "onlyNotVisible", it wraps the main scrollTo in a check for this setting, if provided it checks if the passed target element is within the viewport, if it is then it does not scroll as its already within view range otherwise it does scroll to it.

This extra setting is only to be used with a jquery object or a dom selector id for the target parameter.

This addition adds the extra settings "onlyNotVisible", it wraps the main scrollTo in a check for this setting, if provided it checks if the passed target element is within the viewport, if it is then it does not scroll as its already within view range otherwise it does scroll to it.

This extra setting is only to be used with a jquery object or a dom selector id for the target parameter.
@flesler
Copy link
Owner

flesler commented Dec 11, 2016

Hi @lilpug, I appreciate the time you took to write a pull request. There are reasons why I can't accept the PR:

  1. Totally fixable, you used different indentation and didn't follow other implicit conventions like braces on top. I don't care about which one but I do about consistency with the rest of the code base.
  2. This is more critical, this feature was requested a few times. While useful and somewhat related to the plugin, I believe it's outside the scope of this plugin. You can place the conditional outside the plugin and not call it, the only reason to include it here is convenience, they can be 100% separated code bases. There are several plugins that deal with that checking above/below the fold and doing things only if above/below/within. I think it's better to compose several small plugins instead of having monolithic ones.

For this reason (number 2, really), I'll reject the feature. Hope you understand the reason behind that, thank you again for taking the time to write the code.

@flesler flesler closed this Dec 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants