communities carousel: rewrite React to jinja & jquery #966
Conversation
jennur
force-pushed
the
community-carousel-refactor
branch
from
003c065
to
2a9c2d6
Compare
jennur
force-pushed
the
community-carousel-refactor
branch
5 times, most recently
from
e37557f
to
074b7b9
Compare
There was a problem hiding this comment.
minor: JS-noob question: doesn't all this need to be wrapped in a $(document).ready(function () { ... });? Or are we fine since the <script> tag is at the end of the page?
There was a problem hiding this comment.
Indeed, the script tag will not be added at the end of the page, but inside the macro, which could be added anywhere. If we can remove JS completely, even better :)
There was a problem hiding this comment.
minor/nit: do we have a convention for splitting each macro in its own separate .html file? E.g. would this be useful for someone that wants to override the macro/template?
There was a problem hiding this comment.
We don't have a convention as far as I know, I guess it's more personal preference. I find it easier to navigate this way, but if there are arguments against it, let me know. (I think the overriding should be easy either way)
| import ReactDOM from "react-dom"; | ||
| import CommunitiesCarousel from "./CommunitiesCarousel"; | ||
| import { OverridableContext, overrideStore } from "react-overridable"; | ||
| import $ from "jquery"; |
There was a problem hiding this comment.
annoying guy mode on :)
Can we get rid of all the JS and use CSS only? Example first link searching on the internet...
There was a problem hiding this comment.
Agree on that if it's possible.
An alternative, in case it's too difficult/tricky to get right, would be to optimize for how the page loading looks like with "Slow 3G" throttling. If we get the first carousel "slide" to display ok, and get the interactivity later, maybe it's good enough? That's how it currently works for the legacy Zenodo frontpage.
There was a problem hiding this comment.
Tip: to achieve that, we can try to avoid any dependency and simply use plain JS (zero imports, no jquery). In this way, we will avoid depending on any bundle, and the JS can be downloaded and executed independently of other bundles.
There was a problem hiding this comment.
Testing with fast 3g (slow 3g takes forever to show anything at all - just a white page) the template is loading equally fast as the temporary "recent uploads" placeholders, but without any functionality for a while, so I guess that's what you suggested already?
All the html is rendered with jinja, the jQuery is only used for the functionality of it. With fast 3g the js-file takes 881ms to load as it is now (vs other bundles taking ~2 minutes). Removing it doesn't make much difference, the page still takes 2.1 mins to load.
There was a problem hiding this comment.
I'm a bit surprised that even the slow 3g doesn't render anything for a while, even the unstyled/raw HTML should appear quite quickly. Though I see the main CSS bundle is pretty hefty (~500KB), and blocks. I think we can't avoid loading the larger global base theme JS/CSS chunks, which from what I understand might be bundled with other things as an optimization (unless we start messing with the Webpack chunking config).
Maybe the best way to go ahead is to deploy with this initial fix, and then open a separate task to optimize the CSS (which is the one that causes the blank white page), with the aim that we get the HTML + some critical CSS to load first.
| communitiesCarouselContainer | ||
| ); | ||
|
|
||
| const carouselSlides = $(".carousel.transition"); |
There was a problem hiding this comment.
would it be possible to use # instead of .? Otherwise, if .carousel.transition is a child of an id, $("#id").find(".carousel.transition") is much more performant. (I used jQuery here, but we should do it without.
jennur
force-pushed
the
community-carousel-refactor
branch
7 times, most recently
from
5d778eb
to
8c96a1a
Compare
jennur
force-pushed
the
community-carousel-refactor
branch
from
8c96a1a
to
b56fdd4
Compare
| under the terms of the MIT License; see LICENSE file for more details. | ||
| #} | ||
|
|
||
| {% macro carousel_item(community=None) %} |
There was a problem hiding this comment.
I would keep here just the initial macro and add any new code to any instance that implements this e.g zenodo-rdm
|
Moved to |
Closes #874
Needs inveniosoftware/invenio-app-rdm#2243
Screen recording
jquery-carousel.mov