Sort modules by the specified criteria on #306#315
Merged
felixarntz merged 10 commits intotrunkfrom May 9, 2022
Merged
Conversation
The introduced changes creates a predictable outcome of modules by using the criteria specified in #306 to order the modules first by focus area, then by experimental and lastly by module name, in this particular case the slug was used as the name due the slug is normalized into a lowercase strings. Fixes #306
mitogh
added
[Type] Bug
felixarntz
reviewed
Apr 25, 2022
Member
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mitogh Looks great, thanks! Just a few minor things worth improving.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Add warngins back to the terminal in the scenario where a module is lacking a set of the required properties. Additionally fixed the fact that the `undefined` check was done against the object and would result in false values.
Member
|
@eugene-manuilov Can you please review this so that we can merge it soon? |
eugene-manuilov
requested changes
May 3, 2022
Contributor
eugene-manuilov
left a comment
eugene-manuilov
left a comment
There was a problem hiding this comment.
Almost looks good to me. Left a comment and a nit-pick suggestion. Cris, please, take a look.
Member
Author
|
Thanks @eugene-manuilov would address your PR feedback over the weekend. |
Co-authored-by: Eugene Manuilov <manuilov@google.com>
…:WordPress/performance into fix/306-order-modules-by-custom-criteria
Based on the feedback a single warning is displayed when multiple problems exists in the module order.
eugene-manuilov
approved these changes
May 9, 2022
Contributor
eugene-manuilov
left a comment
eugene-manuilov
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks, @mitogh. Added one more suggestion that is just a nitpick and shouldn't block this PR. Approved.
Co-authored-by: Eugene Manuilov <manuilov@google.com>
Member
|
@eugene-manuilov @mitogh Can one of you please address the JS lint error here? Update: Fixed it below. |
felixarntz
added
the
no milestone
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The introduced changes creates a predictable outcome of modules by using the criteria specified in #306 to order the modules first by focus area, then by experimental and lastly by module name, in this particular case the slug was used as the name due the slug is normalized into a lowercase strings.
Fixes #306
Relevant technical choices
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.