Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve loading method for block styles #28358
Improve loading method for block styles #28358
Changes from all commits
913a27b
434b7c6
fcd9389
c207726
137b7cf
2de9db9
f734c09
04f05a8
a99f443
0a079f0
c7d3dea
b35e6e7
c8f22c2
6af3170
60a0be0
4d10158
d22202c
b0ad350
6536df8
253f273
a71ca1f
ed35fa8
bedf8e1
8f681d0
016462e
74e25fa
1219cae
d222b39
c493c9a
bcc0c15
af53a2e
22045e8
f3f0705
be984b7
c9caa4f
91055b4
ba2ef33
ef9216a
fb90254
dba0939
836e489
79b2635
68d9956
716031c
9309de8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There's a bug in this function: You can easily see it by checking the "button" block in the "emptytheme". You'll see that the paddings are broken in the frontend.
While we could probably fix this somehow by improving the regex, I wonder if it's actually better to not minify ourselves and instead let the minification tools do the work by:
1- Assuming that the file is already minified (and add it to our setup)
2- Introduce a potential convention: load
.min.css
if the file is available instead of.css
.I personally lean towards 1 and potentially support for core blocks falling back in the blocks registration to unminified files if SCRIPT_DEBUG is true. (I don't like smart behaviors in general and we should better let the right tools handle the right job :) )
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.
I guess it's fixed by #29554 that said I still think we'd better removing the minification from the framework itself.
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.
Regarding the bug, #29554 was just merged and fixes it.
I agree however that we should provide minified files, the current ones are OK for debugging but on a real production environment, they are wasteful.
@youknowriad do you know how to do that? I remember I tried to do it in #25220 but ran into issues, mostly because I don't quite understand our tooling and how to use it 😅
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.
I think it's all in the
stylesTransform
function we have in the webpack config which in theory should generate minified styles for production code (npm run build
) and unmagnified ones for development (npm run dev
)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.
Huh... So it already runs as it should, I just always run dev so didn't see that they get minified on production builds 🤦
I'll submit a PR to remove the minify function 👍
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.
At some point in the future, we might want to ship both files at the same time and introduce the
SCRIPT_DEBUG
flag support but it's not only about these blocks styles, it's about all JS and styles in Gutenberg plugin.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.
This is how it works in the WordPress core.
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.
PR submitted on #29624 👍
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.
Related issue about shipping
.min.*
files: #23960There 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.
It's so complex that it could have more unit tests. We can also grow the coverage if any issue is ever discovered.