add array support for DependsOn #91
Conversation
|
I'm not sure if I like this solution. I will start a review and let you know. |
| continue; | ||
| } | ||
|
|
||
| if (is_object($resource->{$dependency['property']}) && is_array($dependency['value'])) { |
There was a problem hiding this comment.
Could you give an example in which scenario is_object($resource->{$dependency['property']}) is used?
There was a problem hiding this comment.
This is used for when a dependency is on a BelongsTo. I have dependency container that is activated by the selection of a BelongsTo field. In that scenario, an object is passed here. You need to compare the id of that object to the passed in array of ids to match.
| } | ||
| } | ||
|
|
||
| if (is_iterable($resource->{$dependency['property']})) { |
There was a problem hiding this comment.
Could you give an example in which scenario is_iterable($resource->{$dependency['property']}) is used?
There was a problem hiding this comment.
This should match the opposite condition for when you want to match a value to an array of values. i.e. multiselect. can handle multi to multi case if needed also.
|
Any update about this PR? |
| return; | ||
| } | ||
| if (Array.isArray(dependency.value)) { |
There was a problem hiding this comment.
I think instead of adding a whole other case to worry about, we should just have the normal dependsOn case singular value just be upgraded to support arrays as values. I think there's also some misses here, specifically on L127, you are using dependency.value but you don't check if it exists until L129 which would make me ask 1. Why check after you are assuming it's there? 2. If we can assume it's there, then why are we checking it at all?
| if (Array.isArray(dependency.value)) { | ||
| if (typeof this.dependencyValues[dependency.field] === 'object' && dependencyValue !== null) { | ||
| if (dependency.hasOwnProperty('value') && dependency.value.includes(dependencyValue.id)) { |
There was a problem hiding this comment.
Based on how similar L129 conditional and L134 conditional are, can we find a way to combine those so that we aren't duplicating the conditional in 2 places? The only difference seems to be what we are seeing is included, dependencyValue.id vs just dependencyValue. All that duplication just means more code to manage and debug when something goes wrong
| } | ||
|
|
||
| if (is_object($resource->{$dependency['property']}) && is_array($dependency['value'])) { | ||
| if (array_key_exists('value', $dependency) && in_array($resource->{$dependency['property']}->id, $dependency['value'])) { |
There was a problem hiding this comment.
Similar here, you are already assuming $dependency has value as an array item, so is the check on L156 broken or are we safe to assume it will always be there?
| } | ||
| } | ||
|
|
||
| if (array_key_exists('value', $dependency)) { |
There was a problem hiding this comment.
Same sort of comment as above, let's just expand the current dependsOn logic to be able to support arrays instead of duplicating it almost completely.
|
using an array instead of a single value would be a nice feature to have. I have a use case today that actually would benefit from this. |
|
Is this stable? |
6 participants
Can we please add array support in the core library? I know a lot of people want it/use it. It would be nice to have it in the master branch and not have to maintain a fork.