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.

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

Feature/81 depend on several fields #82

Merged
merged 5 commits into from May 12, 2021
Merged

Feature/81 depend on several fields #82

merged 5 commits into from May 12, 2021

Conversation

frschi
Copy link
Contributor

@frschi frschi commented Jan 27, 2021

  • added laravel default editorconfig
  • added new functionality for multiple depend on fields at the same time
  • removed unused dependKey from NovaBelongsToDepend.php

is backwards compatible with the old syntax

beforeDestroy() {
Nova.$off("nova-belongsto-depend-" + this.field.dependsOn);
if (this.field.dependsOn) {
Copy link
Contributor

@cwilby cwilby Apr 6, 2021

Choose a reason for hiding this comment

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

This should support both string and array inputs for dependsOn.

if (this.fields.dependsOn instanceof Array) {
  // incoming changes
} else {
  Nova.$off(`nova-belongsto-depend-${this.field.dependsOn}`);
}

created() {
if (this.field.dependsOn) {
Nova.$on("nova-belongsto-depend-" + this.field.dependsOn, async (dependsOnValue) => {

let $busEvents = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

@cwilby cwilby left a comment

Choose a reason for hiding this comment

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

Added some recommendations to support backwards-compatibility

}
});
}
},

methods: {
updateDependsMap(dependsOnValue) {
let exists = false;
Copy link
Contributor

@cwilby cwilby Apr 6, 2021

Choose a reason for hiding this comment

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

Can be refactored to

updateDependsMap({ value, field: { modelClass, modelPrimaryKey } }) {
    const entry = this.field.dependsMap.find(e => e.key === modelClass);

    if (entry) {
        entry.value = value[modelPrimaryKey];
    } else {
        this.field.dependsMap.push({
            key: modelClass,
            value: value[modelPrimaryKey]
        });
    }
},

{
$this->dependsOn = Str::lower($relationship);
foreach ($classNames as &$value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Continuing the effort to make this a non breaking change, if $classNames has one entry, assign it to dependsOn, otherwise create and assign $classNames

@Sergej-Tihonov
Copy link
Contributor

@cwilby thank you very much for your feedback and suggestions. I will apply them until the end of this week.

@Sergej-Tihonov
Copy link
Contributor

@cwilby I have checked you suggestions in detailed. I have applyed them and taked a look at the end result.
I see that you want to keep all the old logic working as it was and apply only the new one on top.

Sorry, but I am agains that change.
My problem with that is, that this way we have to split the logic in two (string & array) in each place.
It's increase the complexity and we can avoid it.

The "string" path is unnecessary as we can handle this case very good with the "array" as well.

As you already highlighted in src/NovaBelongsToDepend.php
We get always an array here. So in the FormField.vue we can assume to have always an array, too.

There is one rare case, that I see where this can be not the case.
If someone does not use the "dependsOn(string ...$classNames)" method and access directly the field "$this->dependsOn"
It that case you run in some problems as you get not an array there.
We can throw an exception if(this.fields.dependsOn instanceof Array) is not an array, to make in obvious.

@cwilby
Copy link
Contributor

cwilby commented Apr 12, 2021

@Sergej-Tihonov thanks for jumping in again 👍

I see your point, src/NovaBelongsToDepend.php uses params which wouldn't be a breaking change. No need to change that as I mentioned.

I don't think dependsOn is supposed to be directly reassigned, I agree with an exception if it's not an array. It would be nice however to normalize dependsOn to an array when it's referenced - is this possible?

$this->dependsOn = is_array($this->dependsOn) ? $this->dependsOn : [$this->dependsOn];

Also curious - why was this block removed?

-        if ($this->dependsOn) {
-            $this->dependKey = $resource->{$this->dependsOn}()->getForeignKeyName();
-        }

Thanks for pushing this forward!

@orlyapps orlyapps merged commit ed984ba into orlyapps:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants