-
Notifications
You must be signed in to change notification settings - Fork 614
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Enhanced Sorting by Nested Meta Values in Pages #681
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.
Thank you for your PR! 👍
LGTM besides some minor things.
Since it's fully backwards compatible we don't need any changes in PicoDeprecated
.
lib/Pico.php
Outdated
$bMeta = isset($bMeta[$key]) ? $bMeta[$key] : null; | ||
} | ||
|
||
$aSortValue = $aMeta; |
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.
Can't we use a single variable here?
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.
I had to think a bit about what you mean exactly. But it should be simpler and still quite clear.
Thank you for the opportunity to contribute here! It's my first time contributing publicly.
lib/Pico.php
Outdated
@@ -1830,11 +1830,22 @@ protected function sortPages() | |||
if ($orderBy === 'meta') { | |||
// sort by arbitrary meta value | |||
$orderByMeta = $this->getConfig('pages_order_by_meta'); | |||
uasort($this->pages, function ($a, $b) use ($alphaSortClosure, $order, $orderByMeta) { | |||
$aSortValue = isset($a['meta'][$orderByMeta]) ? $a['meta'][$orderByMeta] : null; | |||
// Split the configuration value into an array of keys to allow sorting by nested meta values. |
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.
The code is pretty self-explanatory, I don't think that we need inline code comments here (same with the comment below). However, we do need documentation in config.yml.template
and CHANGELOG.md
.
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.
I have removed the comments. I wasn't quite sure if the comments were right here as good style or not.
I am thrilled to hear that my contribution is well-received – it's truly gratifying to see my code being integrated and knowing it will be used by others. Thank you very much for your positive feedback! 😊 Regarding the changelog, I wanted to suggest the following line for inclusion to accurately reflect the new functionality introduced: Suggested Changelog Entry:
I apologize for the earlier commit and revert commit confusion. To avoid further complexities, I thought it best to propose the changelog update here. If you'd prefer a separate pull request for this, please let me know, and I'll be happy to arrange that. Thank you once again for your support and for fostering such a collaborative environment. |
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.
LGTM 👍
See the minor suggested change below. Your suggested changelog addition looks good, too. Can you please add it to this PR? Will merge it then. Thanks!
config/config.yml.template
Outdated
@@ -30,7 +30,7 @@ twig_config: # Twig template engine config | |||
# | |||
date_format: %D %T # Pico's default date format; | |||
# See https://php.net/manual/en/function.strftime.php for more info | |||
pages_order_by_meta: author # Sort pages by meta value "author" (set "pages_order_by" to "meta") | |||
pages_order_by_meta: author # Sort pages by meta value "author" (set "pages_order_by" to "meta"); use '.' for nested meta keys (e.g., 'author.info') |
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.
pages_order_by_meta: author # Sort pages by meta value "author" (set "pages_order_by" to "meta"); use '.' for nested meta keys (e.g., 'author.info') | |
pages_order_by_meta: author # Sort pages by meta value "author" (set "pages_order_by" to "meta") | |
# Use '.' notation for nested meta keys (e.g. 'author.info') |
…cess nested metadata with a dot as separator for sorting.
…etadata with a dot as separator for sorting.
A little tinkering led to success: The changelog has now been changed accordingly! Thanks again for your patience! |
Looks like there went something wrong, the line wrapping in |
Thanks again for your patience! It should be correct now, right? |
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.
LGTM, great work, thank you very much! ❤️ 🎉 👍
Don't worry about the failing CI btw... 😉
This pull request introduces an enhancement to the sorting functionality within pages. Previously, sorting was limited to top-level meta values. With this update, the sorting logic now supports nested meta values, providing greater flexibility and precision in organizing content.
Key Changes:
This enhancement ensures a more dynamic and versatile approach to content organization, particularly beneficial for complex page structures with deeply nested meta data.