DependsOn array values for matching multiple different values#56
DependsOn array values for matching multiple different values#56drsdre wants to merge 8 commits intoepartment:masterfrom
Conversation
To make it easier to compare to multiple values in dependsOn instruction, the function has been altered to accept also an array of values. It still supports single value also.
Example added for matching with an array of values.
Need to reverse the logic from finding false to finding true whilst matching against multiple values.
drsdre
changed the title
drsdre
changed the title
Reverse comparison logic to make chained dependsOn work using coersion.
|
Please merge this! |
| for (let value of dependency.values) { | ||
| // Use coercion as form values are always string | ||
| if (this.dependencyValues[dependency.field] == value) { | ||
| valuesSatisfied = true; |
There was a problem hiding this comment.
a nice optimization here might be to return as soon as we find a value satisfied. There is no point to check other conditions if one matches. Also you could instead use the js includes method to make this even cleaner instead of a local variable (valuesSatisfied) and the for loop
There was a problem hiding this comment.
@ragingdave that’s basically the only problem I have with this package. When chaining multiple dependsOn conditions, it will always resolve in an or statement, when dependencies get resolved as soon as one statement resolves to be true.
There was a problem hiding this comment.
While I agree with you, and might have even said so in an issue about this same topic, that level of improvement for this package seems a very far ways off. I mean this with no disrepect, but clearly you don't have the time for that big of an upgrade, and quite frankly neither do I. Perhaps this particular PR, would be valuable to further improve the 1.x line, with perhaps the query builder like syntax/chaining being something for a v2 or beyond.
|
This is actually far closer to what I would expect to add this than the other PR, so if you make those changes I think we can get it merged, just make sure that you resolve the merge conflicts first. |
|
@drsdre, are you considering making those couple optimizations so this PR can get merged? There seems to be many interested in getting that array support into the native package. |
|
I'm not doing active Laravel Nova projects at the moment. Maybe over the summer I have some time. So if anyone want's to give this a go, let me know and I'll help to get it merged in this PR. |
5 participants
Added ability to match on different values using an array in dependsOn function. Backwards compatible with original single value matching (accepts both single value and array of values).