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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added omittable steps #44

Merged
merged 8 commits into from
Jun 23, 2022
Merged

Added omittable steps #44

merged 8 commits into from
Jun 23, 2022

Conversation

ercsctt
Copy link
Contributor

@ercsctt ercsctt commented May 17, 2022

Issue reference: #43

Please let me know if there's anything I can change here, I've tried to implement this in the most elegant way without having to modify a bunch of logic. I needed this for a personal project, and it works fine as is (afaik), but if there's a better way of doing this without rewriting the entire wizard logic let me know 馃槀

@ksassnowski
Copy link
Collaborator

I took a quick glance at the changes and I think I agree with the overall design. I think availableSteps needs to be cached somehow, so we aren't looping over the steps over and over again. Especially since the omit method of a step can contain arbitrary code (like a network call). I also think that we should pass the current request to the availableSteps method.

I'll have a closer look at this in the next few days

@ercsctt
Copy link
Contributor Author

ercsctt commented May 18, 2022

Unsure if caching would work in this case, as the point of availableSteps is to get any available steps at that time, and be reactive to showing different steps based on the result of omit.

@ksassnowski
Copy link
Collaborator

I meant caching for the current request. For instance, both the summary and nextStep methods currently call availableSteps. Since the response renderers will likely want to call the summary method, we'd essentially be doing double the work for no reason.

I think availableSteps should be lazily calculated and return the cached value if it has already been computed before. Might have to think about invalidating the cache, i.e. force a recompute, after the wizard's data has changed.

@ercsctt
Copy link
Contributor Author

ercsctt commented May 18, 2022

Yep I get that, okay so out of sheer laziness I've used the spatie/once package for this purpose as it does a pretty good job at this with little code change. It's reduced the calls to omit from 3 to 1 in a single request.

@Wulfheart
Copy link

This feature would be really nice indeed.

@ksassnowski
Copy link
Collaborator

I鈥檒l look at this once I鈥檓 back from vacation

@ksassnowski
Copy link
Collaborator

I've made couple of small changes to the PR. Most notably, I implemented the memoization of the available steps myself as I would rather not pull in a whole other dependency just for that. I also added a few extra tests to for the wizard and wizard repository.

Other than that, looks good to me. Thanks for the contribution!

@ksassnowski ksassnowski merged commit 059e66d into laravel-arcanist:main Jun 23, 2022
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

3 participants