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

Update module directories to be within their focus directory #58

Merged
merged 21 commits into from
Dec 30, 2021

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Dec 21, 2021

Addresses #46

  • Move the webp-uploads module into a images folder in modules.
  • Update perflab_get_modules to parse through all the focus folders to look for modules.
  • Modules stored in settings now include their focus as part of the module name, for example webp-uploads changes to images/webp-uploads - for this reason, existing users will need to re-enable the webp-uploads module. Not a big concern since only developers are testing this so far.
  • Update the docs to reflect the new file/folder structure for modules.
  • Remove Focus: from the headers of all sample code and from the webp-uploads module header (only change to module itself)
  • Add logic in perflab_get_module_data to use a regex to extract the module type from the module filename (which includes the focus slug as part of its path). Feedback on regex always welcome!

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.

@adamsilverstein This looks good except for the logic change for parsing modules, which is now coupled to which focus areas we have defined and also causes tests to fail. Can you update that? I believe some related tests need to be updated as well.

Other than that, only minor comments for e.g. a few oversights that still need to be updated.

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
docs/Writing-a-module.md Show resolved Hide resolved
modules/images/webp-uploads/load.php Show resolved Hide resolved
@felixarntz
Copy link
Member

@adamsilverstein Also, please add the appropriate PR labels and milestone based on #52.

@adamsilverstein adamsilverstein added [Focus] Images [Type] Enhancement A suggestion for improvement of an existing feature labels Dec 21, 2021
@adamsilverstein
Copy link
Member Author

Thanks for the feedback @felixarntz - I'll work on addressing these points.

@adamsilverstein
Copy link
Member Author

Addressed feedback, updating tests next to work with new setup.

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Dec 21, 2021

@felixarntz ready for review:

  • module loading now parses all folders in modules
  • moved all module tests into focus folders
  • used module_dir variable and store slug in module_data
  • ensure all tests are updated to pass

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.

@adamsilverstein Almost lgtm now! Only a few last minor comments.

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Show resolved Hide resolved
docs/Writing-a-module.md Outdated Show resolved Hide resolved
docs/Writing-a-module.md Outdated Show resolved Hide resolved
tests/admin/load-tests.php Outdated Show resolved Hide resolved
@felixarntz felixarntz changed the title Move webp-uploads into modules/images focus folder Update module directories to be within their focus directory Dec 22, 2021
@felixarntz
Copy link
Member

@adamsilverstein Note there is also #60 being implemented right now. Depending on which of these two PRs is merged first, the other one may need an update.

@felixarntz
Copy link
Member

@adamsilverstein I've pushed two small commits here just to rebase and adjust the code from #60 to work with this, since that was just merged (see #58 (comment)).

adamsilverstein and others added 4 commits December 30, 2021 09:21
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@adamsilverstein
Copy link
Member Author

@felixarntz - ready for another review when you have a chance

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.

@adamsilverstein Excellent work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants