-
-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inline CSS options #190
Inline CSS options #190
Conversation
Hello @bigwillch, thanks for your work, you made a great job! I have only one problem: using Is it possible? |
Thanks for the feedback! I'll have a a crack at creating new async methods as suggested |
Thanks, let me know when it is done! |
@neoziro just a question - would you be opposed to the package providing both sync and async methods so that the developer can choose which method to use in the app? I'm just thinking that the tradeoff between performance and having to wrap the html output in promises may be something developers would like to be able to choose themselves |
@bigwillch yes we could expose the two methods, just add |
@neoziro I've changed the functionality to use
Thanks again for your feedback so far! |
I am not OK to return a different type (especially a promise) when a custom option is passed to the function. I think it is not clear for the user, it can be confusing and it is not type friendly. Can you please create a different method for each one? |
@neoziro no problem :) Have made them separate methods as requested |
I'm very interested in this functionality :) I can help testing it on my project let you know if I have any issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So finally you decided to no add the sync way? OK for me, it is an anti-pattern for production. Let's wait for issues.
@bigwillch can you please fix the resolve / reject and I will merge it! Thanks! |
@neoziro thanks for flagging! sloppy of me :) Hope it's all ok now 🙏 |
Thanks for your work @bigwillch! |
Great! Thanks for approving |
@neoziro has v5.3.0 been published to npm yet? I'm not seeing it when trying to upgrade |
v5.3.0 & v5.4.0 are published. See https://www.npmjs.com/package/@loadable/server. |
sorry, I'm dumb. Was trying to upgrade loadable/component instead. doh |
I am going through the code and I can’t find where do you declare which style elements you want inline? I dont want to include the entire apps css inline, just the part above the fold. |
@nesbtesh - there is no such thing as "above the fold", without prerendering your application into some virtual window first - then you'll know what it above the fold. You are probably looking for used-styles in stream mode, where styles are inlined before they used, no matter of the "fold". |
@theKashey that worked great, thanks, time to interactivity improved to 1s |
Hi. We have a requirement to render our css inline in certain situations, and so I had a crack at implementing it with the loadable library. Please do let me know what you think about the approach.
Summary
In order to improve page speed score, or to render AMP SSR pages alongside the main app, it can be a requirement to render css inline.
Test plan
This has been added as an option to the existing
chunkExtractor.getStyleTags()
andchunkExtractor.getStyleElements()
functions via aninline
boolean parameter.Additionally a
chunkExtractor.getCssString()
function has been added to allow developers further control over how to render css (an example use requirement being passing the css string to a style tag using AMP'samp-custom
attribute)Appropriate tests have been added to
ChunkExtractor.test.js