Add CSS content support - Fixes #319 #324
Conversation
|
Note this is probably a major breaking change since I changed the params sent into |
There was a problem hiding this comment.
Looking great already! I have one concern regarding the loading of the CSS.
In general I think the README could use a small update to explain the different was of including CSS as the css-content config you introduced might be valuable for others as well. Do you think you can pick that up?
| { | ||
| $filesToInline = $this->config['css-files'] ?? []; | ||
|
|
||
| $this->cssToAlwaysInclude = $this->loadCssFromFiles($filesToInline); |
There was a problem hiding this comment.
If both functions are used to add to the $this->cssToAlwaysInclude perhaps it would be best to also use the .= here as then the order of method calling has no side effects.
| $this->cssToAlwaysInclude = $this->loadCssFromFiles($filesToInline); | |
| $this->cssToAlwaysInclude .= $this->loadCssFromFiles($filesToInline); |
| if (! $contentToInline) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggestion: Insert this line
if ($contentToInline instanceof \Closure) $contentToInline = $contentToInline();
In most cases, the css content will only be used (and this code reached) when an email is being prepared for sending, which may be no more than 1% of page loads depending on the project. So for better performance, lazy loading should be allowed.
With the line above inserted, instead of passing Vite::content($asset) directly, we can pass fn() => Vite::content($asset).
DannyvdSluijs
force-pushed
the
master
branch
5 times, most recently
from
6f842ab to
607634d
Compare
|
Closing PR for now due to lack of response from creator. @Muffinman feel free to reopen the PR if more information is available. |
@DannyvdSluijs for your approval.