Skip to content
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.

Sign up for GitHub

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

add array support for DependsOn #91

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+33 −3
Open

add array support for DependsOn #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16638b5
add array support for DependsOn
dtannen Oct 25, 2019
349ac94
remove or matching
dtannen Oct 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class Page extends Resource

The package supports four kinds of dependencies:

1. `->dependsOn('field', 'value')`
1. `->dependsOn('field', 'value')` or `->dependsOn('field', ['value1', 'value2'])`
2. `->dependsOnEmpty('field')`
3. `->dependsOnNotEmpty('field')`
4. `->dependsOnNullOrZero('field')`
Expand Down
2 changes: 1 addition & 1 deletion dist/js/field.js
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 14 additions & 0 deletions resources/js/components/FormField.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,20 @@
return;
}

if (Array.isArray(dependency.value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (typeof this.dependencyValues[dependency.field] === 'object' && dependencyValue !== null) {
if (dependency.hasOwnProperty('value') && dependency.value.includes(dependencyValue.id)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.dependenciesSatisfied = true;
return;
}
} else {
if (dependency.hasOwnProperty('value') && dependency.value.includes(dependencyValue)) {
this.dependenciesSatisfied = true;
return;
}
}
}

if(dependency.hasOwnProperty('value') && dependencyValue == dependency.value) {
this.dependenciesSatisfied = true;
return;
Expand Down
18 changes: 17 additions & 1 deletion src/NovaDependencyContainer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,23 @@ public function resolveForDisplay($resource, $attribute = null)
continue;
}

if (is_object($resource->{$dependency['property']}) && is_array($dependency['value'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give an example in which scenario is_object($resource->{$dependency['property']}) is used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (array_key_exists('value', $dependency) && in_array($resource->{$dependency['property']}->id, $dependency['value'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

$this->meta['dependencies'][$index]['satisfied'] = true;
continue;
}
}

if (is_iterable($resource->{$dependency['property']})) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give an example in which scenario is_iterable($resource->{$dependency['property']}) is used?

Copy link
Author

@dtannen dtannen Oct 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$filtered = $resource->{$dependency['property']}->filter(function ($value, $key) use (&$dependency) {
return $value->id == $dependency['value'];
});
if (array_key_exists('value', $dependency) && $filtered->isNotEmpty()) {
$this->meta['dependencies'][$index]['satisfied'] = true;
continue;
}
}

if (array_key_exists('value', $dependency)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if($dependency['value'] == $resource->{$dependency['property']}) {
$this->meta['dependencies'][$index]['satisfied'] = true;
Expand All @@ -165,7 +182,6 @@ public function resolveForDisplay($resource, $attribute = null)
continue;
}
}

}
}

Expand Down