MU plugin for the Server-Timing API#7
Conversation
mukeshpanchal27
added
needs:code-review
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 Looks like this includes the code from #6 and will need to be refreshed once that PR is merged.
But I already took a look at the new code in this PR and left some feedback. Looks almost good to go!
To make reviews easier, you could set the target branch to the setup-initial-performance-action branch and mark this a blocked until after #6 is merged. That way no refactor will be needed and this PR should automatically be updated to point to the feature branch once that one gets merged. |
joemcgill
left a comment
joemcgill
left a comment
There was a problem hiding this comment.
The updates to the mu-plugin look good to me.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 One small point of feedback on avoiding globals where not necessary. That in itself is not necessarily a blocker, but now that #6 was merged, we need to also refresh this branch and resolve the merge conflict, so maybe you can address my new feedback then as well.
Related is @joemcgill's comment from #7 (comment), that way we can avoid such conflicts in the future.
…ver-timing-api-mu-plugin
|
Thanks for the feedback. @joemcgill I have addressed your feedback, which you have asked for here. |
|
Thanks @mukeshpanchal27. The updates look good to me. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 Thank you for making the iterations, LGTM. Tiny feedback below, but not blocking!
3 participants
The PR adds a MU plugin for Server-Timing API and a GH action step that add mu plugin in docker container.
Closes #3