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
introduce filtered pagination #376
base: main
Are you sure you want to change the base?
Conversation
$parts = explode('.', $matches[2]); | ||
$name = $parts[0]; | ||
$expectedKey = $parts[1]; | ||
$expectedValue = $parts[2]; |
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.
You can simplified to this, what do you think?
list($name, $expectedKey, $expectedValue) = explode('.', $matches[2]);
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'm personally not the biggest fan of writing code like this because it reduces readability in my opinion.
I like the code more if it is written in a way that it is possible to read it like a book and not stacking up too many things into one line.
This is just personal taste. Anyways, I don't want to open a discussion on code style so I will just follow any advice.
|
||
$filteredData = array(); | ||
foreach ($data as $key => $page) { | ||
if (is_string($page->meta()[$expectedKey]) && $page->meta()[$expectedKey] === $expectedValue) { |
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.
Is not it necessary to check the existence of the key, like this?
array_key_exists($expectedKey, $page->meta())
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.
As master
is on PHP 7.2 now, both of these approaches could probably be simplified using the null coalesce operator. As an example:
$actualValue = $page->meta()[$expectedKey] ?? null;
if ($actualValue === $expectedValue) {
....
}
However, I'd caution against accepting that code example verbatim - I'm only speaking on a syntactical level, I'm not currently sure if the value null
is an appropriate default to use there.
I'm not 100% sure what this would look like in the common use case. Could you provide a more detailed explanation of the goal and what the outputs/advantages would be? I guess I'm not exactly sure what the advantage is here instead of the common usages of tags/categories. Or, to put it another way, I'm wondering if the taxonomies feature (used for tags and categories) could be leveraged to make this functionality more widely applicable. |
Hey Here a bit more about the use case:
On top I build now sub overviews for my trip to OZ 2017
In the last source file, you can see now that I used the filtered option.
In all my blog posts regarding my trip to OZ you'll find this at the top:
The function filtered() will now generate a list of entries which have a I hope now it is more clear what it's used for. |
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 think using the short array syntax is better than array
syntax 😄 :).
|
||
$data = $this->dataProviderManager->dataProvider($name)->provideData(); | ||
if (!count($data)) { | ||
$data = array(); |
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.
How about using the short array syntax? I think this short array syntax is better than array
😄.
$data = array(); | ||
} | ||
|
||
$filteredData = array(); |
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.
It's the same problem and I mention in the previous comment.
I'd like to propose the following feature.
filtered pagination is providing an opportunity to generate an overview page only with blog posts which have a certain meta block provided. It is indifferent if it is part of a list or a simple string.
example
In blog post itself:
in overview page:
On this overview page will now appear only entries with the meta information 'show: this'.
I hope you find this feature helpful and consider to merge it in.