-
Notifications
You must be signed in to change notification settings - Fork 107
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
Conversation
There was a problem hiding this 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.
@adamsilverstein Also, please add the appropriate PR labels and milestone based on #52. |
Thanks for the feedback @felixarntz - I'll work on addressing these points. |
…eas`" This reverts commit 9ecb11a. # Conflicts: # admin/load.php
Addressed feedback, updating tests next to work with new setup. |
d3b818a
to
3c4681a
Compare
@felixarntz ready for review:
|
There was a problem hiding this 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.
@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. |
@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)). |
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@felixarntz - ready for another review when you have a chance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsilverstein Excellent work!
Addresses #46
webp-uploads
module into aimages
folder inmodules
.perflab_get_modules
to parse through all the focus folders to look for modules.webp-uploads
changes toimages/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.Focus:
from the headers of all sample code and from thewebp-uploads
module header (only change to module itself)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!