wdelfuego / nova-wizard

A simple multi-step Wizard tool for Laravel Nova 4
Other
11 stars 5 forks source link

Made it possible to add dependsOn fields #6

Open lagerroos opened 8 months ago

lagerroos commented 8 months ago

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

wdelfuego commented 8 months ago

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

A couple of remarks/doubts I have;

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 commented 8 months ago

@lagerroos Any update? :)

Happy holidays! 🎄

lagerroos commented 8 months ago

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! 🎅🏻

DanOliveira98 commented 4 months ago

Please review and publish the PR.

mlordi commented 3 months ago

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.