-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat: allow multiple filter values, fixes #7328 #7329
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
base: main
Are you sure you want to change the base?
Conversation
@paulroub, thanks for your contribution. Before we review, can you please ensure the tests are passing? |
e507ff3
to
ed53741
Compare
@martinjagodic Fixed the linting issues, along the way had to tell stylelint to ignore |
74e69f2
to
7300752
Compare
Rebased on current |
7300752
to
e8d3ef2
Compare
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.
Pull Request Overview
Adds the ability for collection filters to match against multiple values by updating the value type, normalizing inputs, and enhancing the filter logic.
- Expand
FilterRule.value
to accept arrays, ImmutableList
, or typed records - Normalize raw filter and field values via a new
valuesAsArray
helper - Update
filterEntries
to use normalized arrays and add a corresponding unit test - Ignore
backend.ts
in Stylelint to prevent lint failures on new code
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/decap-cms-core/src/types/redux.ts | Broaden FilterRule.value type to support multi-value inputs |
packages/decap-cms-core/src/backend.ts | Implement valuesAsArray and update filterEntries for arrays |
packages/decap-cms-core/src/tests/backend.spec.js | Add test covering array-based filter values |
.stylelintrc | Include src/backend.ts in lint ignore list |
Comments suppressed due to low confidence (1)
packages/decap-cms-core/src/tests/backend.spec.js:98
- This test covers plain arrays but doesn't verify behavior when
value
orentry.data[field]
are ImmutableList
orStaticallyTypedRecord<List<string>>
. Add tests for those types to ensure full coverage of the new normalization logic.
it('filters multiple values', () => {
valuesAsArray(rawValue: string | List<string> | StaticallyTypedRecord<List<string>>): string[] { | ||
let values: string[] = []; | ||
|
||
if ((<StaticallyTypedRecord<List<string>>>rawValue).toJS) { |
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 check for toJS
comes before distinguishing Immutable List
instances, so Lists (which also have toJS
) will be treated as records. This leads to invoking .toJS().toArray()
on a plain JS array, causing a runtime error. Consider using List.isList(rawValue)
first, then detect records by checking for toJS
.
if ((<StaticallyTypedRecord<List<string>>>rawValue).toJS) { | |
if (List.isList(rawValue)) { | |
values = rawValue.toArray(); | |
} else if ((<StaticallyTypedRecord<List<string>>>rawValue).toJS) { |
Copilot uses AI. Check for mistakes.
@paulroub it's been a while, I hope this is still relevant to you. When I try this with my test config, the collection fails to load entries - even before I tried the list of filters. See screenshot below. I also have an idea of extending this further by adding an operator like with the Condition example:
Maybe we should combine these functions, as it's potentially the same feature on a different level. ![]() |
I'm so far unable to reproduce this -- any chance you could share your test config? |
Reproduced, fixed, added a test case. We were assuming the presence of the filtered-on field on all
I had considered that as well, and poked at it a bit. I hadn't considered extending the syntax, that would make things much more doable. Perhaps a follow-on? This version seems useful on its own. |
Great, thanks! We can do that in a later PR, I agree. If all is ok now, we should merge this for the 3.9 release. |
Summary
Allows a collection filter to match one of a number of values; still allows the existing single-value syntax, as well as an array of values.
Need to manage a collection that's a combination of existing categories (rather than re-categorize all those posts).
Test plan
Checklist
Please add a
x
inside each checkbox:A picture of a cute animal (not mandatory but encouraged)