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

Feature/ #636 Module Specific Readme Files #664

Conversation

10upsimon
Copy link
Contributor

@10upsimon 10upsimon commented Mar 2, 2023

Summary

Addresses #636

Relevant technical choices

Checklist

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

@10upsimon 10upsimon added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release Creating standalone plugins labels Mar 2, 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.

@10upsimon Can you please limit the changes here to just the WebP Uploads module? Other modules to deploy as standalone plugins will be handled in a separate issue.

.gitattributes Outdated
/modules/images/dominant-color/readme.txt export-ignore
/modules/images/fetchpriority/readme.txt export-ignore
/modules/images/webp-support/readme.txt export-ignore
/modules/images/webp-uploads/readme.txt export-ignore
Copy link
Member

Choose a reason for hiding this comment

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

The requirements state to only add a readme for WebP Uploads as part of this issue. Let's stick to that and ignore all other modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixarntz I've changed this to a single 1 line glob pattern now, based on @joemcgill 's feedback so that we can match current and future readme.txt files

/modules/**/readme.txt export-ignore

Let me know if this is good, or if you want me to specifically define it as webp-uploads only, and single out modules in the future.

@10upsimon
Copy link
Contributor Author

@felixarntz I've addressed your comment RE restricting this to webp-uploads only, however I did implement a catch-all glob pattern match for all module readme.txt files in order to export-ignore them.

Once @mukeshpanchal27 's PR at #662 is approved and merged, I'll enhance the CLI command to handle the copy of the readme.txt files.

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.

@10upsimon Thank you for the updates. I think there is a misunderstanding here, as the standalone plugins will be in fact standalone. They should not be branded/marketed as extensions to the Performance Lab plugin.

Right now, as part of Milestone 1 (see #656), they will exist in both places, as a standalone plugin and as a module in the Performance Lab plugin. But once we're done with Milestone 2, they will only exist as standalone plugin.

Using Performance Lab alongside them is perfectly fine and we would still encourage users to use Performance Lab if they want to follow along what the performance team is working on. But someone that wants to just use WebP would be perfectly fine to only install this plugin on its own.

Regarding the dependency on #662, I don't think we technically have to wait for that PR. Once this PR is finalized, it can be merged as is, since #662 will copy the entire directory already, so the readme.txt here will be included.

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 @10upsimon, I left one minor suggestion, and could you please remove the readme.txt file for sqlite? 

@10upsimon 10upsimon marked this pull request as ready for review March 13, 2023 11:10
@10upsimon
Copy link
Contributor Author

@mukeshpanchal27 @felixarntz many thanks for your feedback and comments. I believe I have addressed them all and have requested a re-review of the PR. I confirm the readme.txt was copied across as expected with @mukeshpanchal27 latest merge of #662 @felixarntz

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 @10upsimon, The changes look good to me.

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
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.

@10upsimon This is pretty much good to go now, just one small thing and one slightly bigger thing. It would be great if you could expand the description just a tiny bit to explain a bit more clearly what the plugin does.

We can also iterate in a follow up if you prefer for just that content, in order to unblock this PR. Let me know what you prefer.

mukeshpanchal27 and others added 3 commits March 15, 2023 15:15
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
…om:WordPress/performance into add/issue-636-module-specific-readme-files
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.

@10upsimon Thank you for the updates, LGTM!

@felixarntz felixarntz merged commit 3e279a6 into feature/creating-standalone-plugins Mar 16, 2023
@felixarntz felixarntz deleted the add/issue-636-module-specific-readme-files branch March 16, 2023 15:54
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 no milestone PRs that do not have a defined milestone for release [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants