Skip to content
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

Add output buffering checkbox to Server-Timing screen #801

Merged
merged 18 commits into from Aug 18, 2023

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 12, 2023

Summary

Implements output buffering checkbox as discussed at #784 (comment).

Relevant technical choices

A new boolean output_buffering array item is added to the perflab_server_timing_settings option. This value, defaulting to false, will be used if there is no perflab_server_timing_use_output_buffer filter present.

Screenshots

When the option is not enabled, the note at the top of the screen remains:

image

When the option is enabled, the note at the top is removed:

image

When a perflab_server_timing_use_output_buffer filter is added which returns true, the checkbox is checked and disabled with a note about the filter being present:

image

The same is true when the filter returns false:

image

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@westonruter westonruter added [Type] Feature A new feature within an existing module Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release labels Aug 12, 2023
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @westonruter, this looks great so far. I left some feedback, most importantly I'm thinking we could display the field in a slightly more prominent way.

admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
server-timing/class-perflab-server-timing.php Outdated Show resolved Hide resolved
@felixarntz felixarntz added this to the PL Plugin 2.6.0 milestone Aug 17, 2023
@felixarntz felixarntz changed the base branch from trunk to release/2.6.0 August 17, 2023 16:32
@felixarntz
Copy link
Member

@westonruter Given that this applies to the newly added screen, I've added this to the 2.6.0 milestone and updated the base branch accordingly, let's try to merge it in time.

@felixarntz felixarntz mentioned this pull request Aug 17, 2023
3 tasks
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Some mostly very minor follow-up feedback.

admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
server-timing/class-perflab-server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Show resolved Hide resolved
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost there. A few last points of feedback.

admin/server-timing.php Show resolved Hide resolved
admin/server-timing.php Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter One final nit-pick on wording, but otherwise this looks great! 🎉

admin/server-timing.php Outdated Show resolved Hide resolved
@felixarntz felixarntz removed the no milestone PRs that do not have a defined milestone for release label Aug 17, 2023
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @westonruter for the PR. LGTM! I left some non blocking feedback.

admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
admin/server-timing.php Outdated Show resolved Hide resolved
tests/server-timing/perflab-server-timing-tests.php Outdated Show resolved Hide resolved
@felixarntz
Copy link
Member

@westonruter Could you take a look at @mukeshpanchal27's feedback please so that we can try to merge this today?

westonruter and others added 2 commits August 18, 2023 10:01
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
@felixarntz
Copy link
Member

@westonruter @mukeshpanchal27 In light of the CSS feedback, after taking a closer look I've pushed an update in 7e8c9c5 that changes the markup to match what is used on WP core's own settings screens, specifically for the uploads_use_yearmonth_folders UI control (which for example still uses a .form-table, which was previously missing here and therefore required several style tweaks to look consistent with other core settings UI). This change also means that almost all of the custom CSS was removed while maintaining a consistent appearance.

@felixarntz felixarntz merged commit 57ca48a into release/2.6.0 Aug 18, 2023
7 checks passed
@felixarntz felixarntz deleted the add/output-buffering-checkbox branch August 18, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants