refactor: Use IntersectionObserver in Layout component #168
Conversation
|
+1 on using the observer, but I'm not sure what is the minimum browser version we are target for our website (because as you mentioned observer wont work in some browsers)? |
|
/gcbrun eventual staging example will be on https://storage.googleapis.com/staging.nodejs.dev/1001a01/index.html |
Yeah, I'd suggest keeping both the implementations, preferring |
|
Thanks for the input @antsmartian, @BeniCheni, @sagirk. I'm holding off with this one until we have merged #157 🙂Nice idea to keep the current implementation as a fallback 👍 |
|
@ollelauribostrom #157 is merged. You may want to rebase against the latest master. |
ollelauribostrom
force-pushed
the
observer
branch
from
1001a01 to
fc4046a
Compare
|
@sagirk Rebased and added back the old implementation as fallback. |
There was a problem hiding this comment.
@ollelauribostrom , LGTM after your rebase and update. 🎊
P.S.: per .querySelector('.side-nav'); and .querySelectorAll('.side-nav__title');. Could refs be used instead of class selector? (curious about your thoughts if a follow-up refactoring would improve; I could tinker around)
|
@BeniCheni @ollelauribostrom https://github.com/nodejs/nodejs.dev/issues/180 opened to continue discussion/work on #168 (review). |
This might need a bit of tweaking so consider it a WIP.
The idea is that instead of calculating the hero offset in each animation frame - we can use the IntersectionObserver API. We are notified each time the intersection changes and when a
.side-nav__titleintersects with the hero banner, we apply some white color. Should offer some performance benefits and might also help solve #86.Seems like the browser support is pretty good, so this should work out of the box in most browsers. What are your thoughts on this?