search

jonkwheeler / ScrollScene

ScrollScene is an extra layer on top of ScrollMagic as well as using IntersectionObserver to achieve similar effects.
https://scrollscene.jonkwheeler.now.sh/
MIT License
136 stars 9 forks source link

`triggerElement` must be optional #30

Closed WisdomSky closed 4 years ago

WisdomSky commented 4 years ago

The triggerElement property must be optional just like the original construct of the ScrollMagic's Scene class.

I'm only using offsets in all of my current implementations and I don't want to do extra work by creating elements for each.

Here's the error when I only try to use only offset: image

And here's the ScrollMagic docs:

Screen Shot 2020-04-19 at 6 51 06 AM
WisdomSky commented 4 years ago

It would be better if you have all the parameters the same as the one on ScrollMagic in order to avoid confusion. We are under the impression that it's the same as ScrollMagic's scene, so we expect the parameters to be the same, except the new ones (e.g. controller).

jonkwheeler commented 4 years ago

This is super rare edge case, but I'll merge it.

jonkwheeler commented 4 years ago

Released v0.0.16

WisdomSky commented 4 years ago

@jonkwheeler

I don't understand why it would be considered a super edge case. How?

Is using offset for adding trigger points an edge case? IMO, not everyone is using triggerElement.

jonkwheeler commented 4 years ago

IMO, it's super edge because I don't know anyone not setting a triggerElement, and if they wanted to use the body as the triggerElement, it would be smarter to just set that for readability across a team. It would be super confusing to me if my teammate had no triggerElement set. Again, that's just my opinion.

WisdomSky commented 4 years ago

Just because you don't know anyone that's not using a triggerElement, doesn't mean it's a super edge case. That's a very biased assumption. I particularly am one of those who don't use triggerElement.

I also think that you're confusing the use case here. We don't use the body as the trigger element either. We are taking advantage of the offset's behavior where we can completely geet rid of the hassle of creating nodes and just set a value instead which will act as the trigger point which in this case the value is the scroll offset.

In ScrollMagic, the triggerElement was optional and the reason for that is to provide the user an alternative ability to set trigger which is via offset. In ScrollMagic's docs it's clearly stated that the triggerElement is optional only if you provide an offset value instead. So how can that be an edge case?

An edge case is when you try to do something that is not actually stated in the docs, but on this one it's clearly stated. The author must've realised that this could be one of common use cases and decided to set it like that.