speed(homepage) offload images until in viewport #2956
Conversation
There was a problem hiding this comment.
I'm not sure about this package. Pls try implementing this natively using Intersection Observer
|
What can i say... If you are not sure, check the source code. |
|
@EugeneHlushko I meant, I am not sure about the viability of the package. Hence I prefer to implement the feature using Intersection Observer API. |
|
Really concerned with browser support around Intersection Observer. I am planning to make a HOC and wrap them into it. |
EugeneHlushko
force-pushed
the
lazy-contribs
branch
from
f0afee6 to
8ba8d6a
Compare
|
Decided to not go with another HOC, rather extend my reusable component ;] @byzyk |
src/components/Support/Support.jsx
Outdated
| @@ -61,9 +62,10 @@ function formatMoney(number) { | |||
| return str; | |||
| } | |||
|
|
|||
| export default class Support extends React.Component { | |||
| export default class Support extends VisibilitySensor { | |||
There was a problem hiding this comment.
2 things:
-
And most important one, this break the goal of this project, @skipjack vision when rebuilding this project is to focus solely on content, meaning that custom plugins/components/logic/code etc should be outside of it, as dependencies, that's why my custom MDX plugins are dependencies.
-
This way of writing React components is not recommended by React itself (https://reactjs.org/docs/composition-vs-inheritance.html#so-what-about-inheritance), meaning that it is not idiomatic, which is important if you want to attract new contributors, especially in an Open Source project like this one.
There was a problem hiding this comment.
this looks like an overkill to me, @skipjack thoughts?
There was a problem hiding this comment.
I agree with @montogeek in this case.
...when rebuilding this project is to focus solely on content, meaning that custom plugins/components/logic/code etc should be outside of it, as dependencies, that's why my custom MDX plugins are dependencies.
Yeah it seems there are already some pretty popular open source packages that do similar things so we should consider those first instead of bloating this codebase with more components:
- https://github.com/joshwnj/react-visibility-sensor
- https://github.com/fkhadra/react-on-screen
- https://github.com/dazld/react-on-visible
Even if none of those work, you could always write this new component as a separate package the same way we've done for other things.
This way of writing React components is not recommended by React itself (https://reactjs.org/docs/composition-vs-inheritance.html#so-what-about-inheritance), meaning that it is not idiomatic, which is important if you want to attract new contributors, especially in an Open Source project like this one.
Yeah I agree with this as well. However, if you use one of the open source packages I listed above then the new component (and inheritance) wouldn't even be necessary.
There was a problem hiding this comment.
I've started this PR with a react-visibility-sensor which doesn't benefit from Intersection Observer. After looking at our user base i was tempted to go ahead and use that. Which seems to be widely supported nowadays.
So i've done a re-work here within PR.
having that said i will move the visibility sensor into a separate package and rework it from inheritance type module into a HOC
There was a problem hiding this comment.
Thanks for dropping by btw @skipjack 👍
| @@ -0,0 +1,47 @@ | |||
| import React from 'react'; | |||
|
|
|||
| export default class VisibilitySensor extends React.Component { | |||
There was a problem hiding this comment.
It is nice component, good work
|
Apparently there is a: https://www.npmjs.com/package/react-intersection-observer |
|
Its updated CC @montogeek |
|
Preview is ready Built with commit efb793d https://deploy-preview-2956--webpackjsorg-pr-previews.netlify.com |
|
I love this bot lol |
|
Preview is ready Built with commit 4b8d081 https://deploy-preview-2956--webpackjsorg-pr-previews.netlify.com |
|
Yeah, this PR works great! |
|
Thank you!! |
|
Cheers |
Because of the terrible load on the homepage visits, i am offloading img loading until supporters are in the viewport.
Related: #1548
Takes one off #1525