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

Made it possible to add dependsOn fields #6

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

lagerroos
Copy link

This PR enables the user to properly utilize the ->dependsOn functionality on the fields included.

Copy link
Owner

@wdelfuego wdelfuego left a comment

Choose a reason for hiding this comment

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

That's a great update, thanks a lot! Will soon test and merge.

I saw some console.logs left over in your Javascript code that should be removed.

Thanks for this pull request!

@wdelfuego
Copy link
Owner

I've been reviewing this pull request, but I'm not sure it's complete.

A couple of remarks/doubts I have;

  • A patch route to /step/{step}/creation-fields is added in the ToolServiceProvider, but I don't see any requests to that route in the front-end code. Is that correct? Or is this being called in the background by existing field functionality in Nova?
  • Is there a reason why you're adding the Patch route through a separate addFieldDefinitionRoute method, rather than adding it to routes/api.php?
  • I see you added :resourceName="'wizard'+computedInstanceUrl" to the component tag in Tool.vue, what is it's role? Is it required?
  • In Tool.vue, in the reloadFromResponse method, I saw this code:
    vue.steps = vue.steps.map((step) => {
        step.fields.map((field) => {
            console.log('load syncFieldEndpoint', field.methods);
            field.computed.syncFieldEndpoint = () => {
                console.log('syncFieldEndpoint', field);
            }
            return field;
        });
        return step;
    });
    
    Unless I'm mistaken, this code only logs some data for debugging, correct? Or is this code responsible for syncing the dependsOn status itself?

If you can answer these questions, I'll happily go ahead testing and merging this functionality :).

PS I saw that you auto formatted the code to do some clean-up, that's great! In the future, it'd be nice to do so in a separate commit, so it's easy to separate logical changes/additions from pure code formatting changes.

Thanks, @lagerroos !

@wdelfuego
Copy link
Owner

@lagerroos Any update? :)

Happy holidays! 🎄

@lagerroos
Copy link
Author

I've been reviewing this pull request, but I'm not sure it's complete.

A couple of remarks/doubts I have;

  • A patch route to /step/{step}/creation-fields is added in the ToolServiceProvider, but I don't see any requests to that route in the front-end code. Is that correct? Or is this being called in the background by existing field functionality in Nova?

  • Is there a reason why you're adding the Patch route through a separate addFieldDefinitionRoute method, rather than adding it to routes/api.php?

  • I see you added :resourceName="'wizard'+computedInstanceUrl" to the component tag in Tool.vue, what is it's role? Is it required?

  • In Tool.vue, in the reloadFromResponse method, I saw this code:

    vue.steps = vue.steps.map((step) => {
        step.fields.map((field) => {
            console.log('load syncFieldEndpoint', field.methods);
            field.computed.syncFieldEndpoint = () => {
                console.log('syncFieldEndpoint', field);
            }
            return field;
        });
        return step;
    });
    

    Unless I'm mistaken, this code only logs some data for debugging, correct? Or is this code responsible for syncing the dependsOn status itself?

If you can answer these questions, I'll happily go ahead testing and merging this functionality :).

PS I saw that you auto formatted the code to do some clean-up, that's great! In the future, it'd be nice to do so in a separate commit, so it's easy to separate logical changes/additions from pure code formatting changes.

Thanks, @lagerroos !

Hey there!

Yes, indeed it is correct that I've added that route. It's to intercept the equidistant logic in nova so that we can return our field definition if the field in question is a wizard field. Without getting too nitty-gritty, this was the best solution I could find.

The reason why I'm adding it in a separate method is so that it's defined under the right URI (nova-api).

The resourceName is to utilize the built-in field functionality and be able to match it to the right field in the backend (triggering the correct dependsOn functionality)

As to your last question, I removed this logic and some other comments.

Auto-formatting from PHPStorm. We should do a bigger clean-up later to get the project up-to-date with PSR-12, code blocks etc. My partner in crime @olliescase probably have more ideas here.

Wishing you a merry Christmas! 🎅🏻

@lagerroos lagerroos requested a review from wdelfuego December 22, 2023 15:12
@DanOliveira98
Copy link

Please review and publish the PR.

@mlordi
Copy link

mlordi commented May 7, 2024

When using this PR I am getting a 404 error the Request URL looks like the following:

nova-api/wizard/wizard/add-appointment/creation-fields?editing=true&editMode=create&field=location_id&component=select.select-field.location_id

Not sure why there is 2 wizard/ in the provided URL.

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.

5 participants