Closed iFlameing closed 3 years ago
Did you check the size of the new chunk? I expect it to be over 1MB gzipped. Perhaps should be loaded after first attemp to use prettier and not on first render of block?
@iFlameing @mtoepfl is correct. It seems the bundle size increased significantly. We have to at least investigate this and make sure the prettifier lib is lazy loaded and does not slow down Volto.
I haven't seen the bundle size, but if this is correctly lazy loaded, I'm not sure I would suggest loading it only when clicking the prettier button. I think it would be ideal to already have prettier loaded when you click the button. Other options could be loading it on hover if you prefer, or loading it when getting close to the button with the cursor.
If our concern is a slow connection here, we would make the prettier button even slower for the user by loading the library on click, since the browser would have to download the big chunk at that time instead of having it already.
Of course it's important to not load it earlier than the moment the Edit component of the Block is rendered.
One thing I don't like is that we don't have proper dependecies/devDependencies separation. I would have expected a change in package.json, but there's none because all dependencies are bunched together and prettier was already declared.
I've built the bundle, to understand the situation:
File sizes after gzip:
443.5 KB (+2 B) build/static/js/8.34a69812.chunk.js
160.42 KB (+826 B) build/static/js/client.f52fdc21.chunk.js
132.22 KB build/static/js/22.1e0890c2.chunk.js
106.98 KB build/static/js/4.0f63f836.chunk.js
97.05 KB (+60 B) build/static/css/8.faa001b1.chunk.css
32.93 KB build/static/js/2.0ef9bb2a.chunk.js
32.06 KB (+22.62 KB) build/static/js/21.34550faf.chunk.js
26.67 KB build/static/js/0.87f3aabe.chunk.js
16.36 KB (+87 B) build/static/css/client.3b4e3cad.chunk.css
14.3 KB (+812 B) build/static/js/10.5744157d.chunk.js
13.51 KB (-14.11 KB) build/static/js/9.d17a7536.chunk.js
10.08 KB (+310 B) build/static/js/16.584e6d91.chunk.js
9.96 KB (-4.34 KB) build/static/js/11.e3b01dc1.chunk.js
9.79 KB (+637 B) build/static/js/15.84539adf.chunk.js
9.45 KB (+1.33 KB) build/static/js/20.d13b24d5.chunk.js
9.17 KB (+710 B) build/static/js/14.a2a24535.chunk.js
9.06 KB build/static/js/plone-volto-components-manage-Widgets-DatetimeWidget.45458c0f.chunk.js
8.47 KB (+3.46 KB) build/static/js/13.5fc6bc23.chunk.js
8.13 KB (+1.39 KB) build/static/js/18.aad407a0.chunk.js
8.12 KB (-13 B) build/static/js/19.a9dcf5c3.chunk.js
6.76 KB (-3.32 KB) build/static/js/17.3fdd6f69.chunk.js
5.02 KB (-4.94 KB) build/static/js/12.e3a63a25.chunk.js
4.14 KB build/static/js/1.9baa586b.chunk.js
3.39 KB build/static/css/4.9a97b7b4.chunk.css
1.76 KB (+10 B) build/static/js/runtime~client.b7179f32.js
1.45 KB build/static/js/plone-volto-components-theme-View-EventView.9877e31c.chunk.js
Done in 43.72s.
As far as I know, all the extra chunks are loaded "on demand" when the component is loaded, so it should be fine. In this screenshot you can see the prettier chunk loaded on demand when I've inserted the HTML block. This is fine, this is what we want.
Some other things that I've noticed:
I think @tiberiuichim removed the confusion regarding lazy loading. I am loading the prettier lazily, it's not included in the main bundle. Regarding lazily load syntax I got surprised when I first saw it, but loadable provide this syntax. If you want to learn visit this link. This is a link I also tested out lazy loading please see the screenshot we are loading prettier when we are loading the HTML block
@iFlameing Maybe you can also take care of prismjs, while you're at it? Cheers!
@tiberiuichim I didn't get it what are you trying to say about prismjs. Sorry!!!
@iFlameing I mean this: https://github.com/plone/volto/blob/4d3977028c7b624cd8ade14af1be5208f05851c6/src/components/manage/Blocks/HTML/Edit.jsx#L9 and line 10
I'm not sure how that would work, though, I saw that loadable has issues when components are not the default export from a module.
Note from @sneridagh: go to the "eye" and back and it should be prettified.
ok
@sneridagh I gave the "eye" icon a shot and it did not prettify anything (by prettify I mean proper indentation mainly).
@tisto because I haven't added code for this. I will do it today :)
@iFlameing @tisto it stopped working somehow... the code is there, take a look: https://github.com/plone/volto/blob/88b6ea08580436f7a3614f07cab52711238acd97/src/components/manage/Blocks/HTML/Edit.jsx#L122
Anyways, this PR seems to address it, and make it more obvious, so it's fine with me.
@tisto @sneridagh it is ready for review. I am attaching a screencast as a demo.
@iFlameing nice! Do we show a hover tooltip "Prettify HTML" over the "P" icon?
Nope!! We are not showing any tooltip for any options. It is explainable through the button. But it will be a nice feature if we implement it.
@iFlameing see https://react.semantic-ui.com/modules/popup/#types-popup
@iFlameing can we polish it with the latest comments (size of the prettify icon, tooltip)? thanks!
@sneridagh @tiberiuichim I have added the tooltip as well as fixed the icon for prettify code
. Please!!! review this once again.
@iFlameing pls fix the merge conflict.
@tisto done. @sneridagh @tiberiuichim Please!!!!! review this.
@iFlameing pls rebase