Make WordPress Core

Opened 19 months ago

Closed 13 months ago

Last modified 13 months ago

#56388 closed enhancement (fixed)

Compiled block styles contain a lot of unnecessary comment blocks

Reported by: aristath's profile aristath Owned by: flixos90's profile flixos90
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.1
Component: Editor Keywords: has-patch needs-testing
Focuses: performance Cc:

Description

All block styles contain ~75 lines of comments which are not needed. For example you can take a look at https://github.com/WordPress/WordPress/blob/6.0.1/wp-includes/blocks/paragraph/style.css
These comments get added to all non-minified stylesheets and they don't make any sense.

Removing these when possible will reduce the overall package size, make debugging easier since there will be significantly less noise when viewing the source, and improve performance when SCRIPT_DEBUG is true, since the loaded assets will be smaller.

Change History (27)

This ticket was mentioned in PR #3095 on WordPress/wordpress-develop by aristath.


19 months ago
#1

  • Keywords has-patch added

#2 @johnbillion
19 months ago

  • Component changed from General to Editor
  • Focuses performance added
  • Keywords needs-testing added

SergeyBiryukov commented on PR #3095:


19 months ago
#3

This appears to modify some files in the wp-includes/blocks directory, causing some tests to fail 🤔

Ensure version-controlled files are not modified or deleted during building and cleaning
diff --git a/src/wp-includes/blocks/site-logo/style-rtl.css b/src/wp-includes/blocks/site-logo/style-rtl.css
index 9062fc..c4546 100644
--- a/src/wp-includes/blocks/site-logo/style-rtl.css
+++ b/src/wp-includes/blocks/site-logo/style-rtl.css

aristath commented on PR #3095:


19 months ago
#4

This appears to modify some files in the wp-includes/blocks directory, causing some tests to fail 🤔

Ensure version-controlled files are not modified or deleted during building and cleaning
diff --git a/src/wp-includes/blocks/site-logo/style-rtl.css b/src/wp-includes/blocks/site-logo/style-rtl.css
index 9062fc..c4546 100644
--- a/src/wp-includes/blocks/site-logo/style-rtl.css
+++ b/src/wp-includes/blocks/site-logo/style-rtl.css

Yes, that is expected. The patch modifies the compilation of block stylesheets to remove comments from them. Looking at the failing tests, the modified stylesheets are what they should be 👍

SergeyBiryukov commented on PR #3095:


18 months ago
#5

If I understand correctly, it might be possible to make the same change in Gutenberg, so that when these CSS files are backported to core as part of package updates, they would no longer be flagged as modified by this PR, and then it can be merged 🤔

#7 @adamsilverstein
17 months ago

If I understand correctly, it might be possible to make the same change in Gutenberg, so that when these CSS files are backported to core as part of package updates, they would no longer be flagged as modified by this PR, and then it can be merged 🤔

Hi @aristath & @SergeyBiryukov - can you clarify, are the changes here needed in core, in Gutenberg (or both?)? I see some linting (?) failures on the Gutenberg PR.

Also, should we go ahead and punt this to 6.2 or is the change safe enough to include in 6.1?

#8 @aristath
17 months ago

Hey Adam!

The reason we added the Gutenberg PR, is because the tests here are failing, and we don't know if it's because of the Gutenberg files being different or not.
The failing test is Ensure version-controlled files are not modified or deleted during building and cleaning, which would suggest that this is the case.

The Gutenberg PR is not necessary per se, it's only there to resolve the failing test in the Core PR.
Ideally, Gutenberg would remain as-is and Core would apply the changes in this PR.
The change is 100% safe for 6.1 - provided we find a way to fix the failing test.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


17 months ago

#10 @audrasjb
17 months ago

  • Milestone changed from 6.1 to 6.1.1

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next minor milestone, 6.1.1.

Ps: if you are a committer and if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

This ticket was mentioned in Slack in #core by cbravobernal. View the logs.


17 months ago

#12 @cbravobernal
17 months ago

  • Version set to 6.1

Can we take a look at this ticket so we can land it in 6.1? Changes required in the editor will be backported on RC3.

Thank you very much!

#13 @SergeyBiryukov
17 months ago

  • Milestone changed from 6.1.1 to 6.1

#14 @SergeyBiryukov
17 months ago

I suppose this can be merged after the package updates for RC3, so that the affected files in wp-includes/blocks.php are no longer flagged as modified.

Version 0, edited 17 months ago by SergeyBiryukov (next)

#15 @desrosj
17 months ago

  • Summary changed from Compiled block styles contain a lot of trash to Compiled block styles contain a lot of unnecessary comment blocks

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


16 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


16 months ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


16 months ago

#19 @hellofromTonya
16 months ago

  • Milestone changed from 6.1 to 6.2

After discussing with other Core Committers (see here in Slack and here in the GB comment), consensus is:

It's too late in the 6.1 cycle with RC3 happening in a few minutes and final release next week. Moving this ticket to 6.2 cycle.

#20 @flixos90
13 months ago

It looks like there has been no updates here since this was moved to the 6.2 milestone. @aristath Is this something we can realistically complete before beta, or should it be moved to the following release?

I see this is filed as a bug, but that's rather stretching it. While this is certainly worth optimizing, I don't think it qualifies as a bug - nothing is broken, as far as I understand. Would you mind changing it to be an enhancement?

#21 @aristath
13 months ago

  • Type changed from defect (bug) to enhancement

#22 @aristath
13 months ago

It looks like there has been no updates here since this was moved to the 6.2 milestone. @aristath Is this something we can realistically complete before beta, or should it be moved to the following release?

I believe we can safely apply this change. Nothing breaks, and the styles simply get optimized.

I see this is filed as a bug, but that's rather stretching it. While this is certainly worth optimizing, I don't think it qualifies as a bug - nothing is broken, as far as I understand. Would you mind changing it to be an enhancement?

Fair point. Done

#23 @flixos90
13 months ago

  • Owner set to flixos90
  • Status changed from new to reviewing

@aristath Sounds good, thanks. I've just reviewed the PR. With another approval, I'd be confident to commit this.

@SergeyBiryukov commented on PR #3095:


13 months ago
#24

Looks good to me 👍

@gziolo commented on PR #3095:


13 months ago
#25

It doesn't change any behavior for production, so we can proceed with it 👍🏻

#26 @flixos90
13 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55193:

Editor: Remove unnecessary CSS comments from compiled styles.

Props aristath, adamsilverstein, SergeyBiryukov, gziolo.
Fixes #56388.

Note: See TracTickets for help on using tickets.