[API change] Make NavigationMode classes stateless

[earshinov]

It allows the library user to easily create an instance of a particular [[NavigationMode]] class and pass it to <aw-wizard [navigationMode]="...">.

See madoar/angular-archwizard-demo#31 (comment)

[madoar]

I'm not sure how helpful this change is to the users. I think that a significant number of users require direct access to the NavigationMode, mostly to initialize the wizard correctly to a certain step in strict mode.

The problem with this PR is that it would require the users to pass the WizardState themselves to the called navigation methods. This would make using the NavigationMode quite demanding.

[earshinov]

What if angular-archwizard provided thin wrappers for these methods in the WizardComponent class?

goToStep(destinationIndex: number, preFinalize?: EventEmitter<void>, postFinalize?: EventEmitter<void>): void {
  return this.navigationMode.goToStep(this.model, destinationIndex, preFinalize, postFinalize);
}

// and so on...

Benefits:

• Users will no longer need to know that there is a navigationMode instance inside the wizard, which you should use for calling navigation methods. The methods will be easily available in the wizard itself. I believe, it is the most convenient option for the user.
• It solves the issue with goToPreviousStep and goToNextStep methods: Refactor the hierarchy of NavigationMode classes. #166 (comment). These methods will be defined in the WizardComponent and implemented using the generic NavigationMode.goToStep. It will be possible to simplify the NavigationMode interface by excluding these methods from it.

What do you think about this approach?

[earshinov]

I have added "[API change]" to the title to emphasize that this PR introduce incompatible changes in user-visible API.

[madoar]

I agree, adding a wrapper for the methods located in the applied NavigationMode is a good idea!
You're right, this would also make it possible to move goToPreviousStep and goToNextStep to a different place.

[earshinov]

@madoar , maybe WizardState will be a better place for wrapper methods than WizardComponent?

[madoar]

The idea behind the WizardState class was to provide a separate entity to contain the state of the wizard including some methods to make accessing and querying it easier. If we move the control methods (i.e. goTo..., etc.) to the WizardState class, we lose this strict division. I'm not sure whether this is a good idea.

[earshinov]

@madoar , Is it OK that in order to query the current step the user would use WizardState (wizard.model.currentStep), but in order to navigate to the previous or next step he/she would need to use the WizardComponent itself (wizard.goToNextStep)? As a user, I would expect to find these strongly related things in a single place.

Also, if we put the methods into WizardComponent, then in order to access wizard.goToNextStep and other methods, WizardComponent itself will need to be injected into NextStepDirective etc. instead of WizardState. Is it OK (just to clarify)?

[madoar]

I understand what you mean, yes when looking at the NextStepDirective the responsibilities of the WizardState and the NavigationMode classes have already begun to blur.

In this case I think we will need to go a bit further and decide which class will have which responsibilities and how we want to expose the encapsulated functionalities to the outside. I believe the easiest way to navigate the wizard from outside is via methods provided by the WizardComponent class. Alternatively I'm also fine with adding the methods to the WizardState class, but not only would this blur the responsibilities of both classes, in addition it would also require the users to use wizard.model.goTo... instead of wizard.goTo.... Still it is debatable whether this is really a bad thing...

Summarized I'm basically fine either way. We just need to specify how the responsibilities of the classes should be split!

[earshinov]

I agree that responsibilities are blurred. Are you sure that we need WizardState class at all?

For example, see this component: https://www.primefaces.org/primeng/#/table (scroll down to Properties section). It exposes everything right in the component class. And that component is larger that angular-archwizard.

Perhaps, we should follow this example and expose all methods and properties right in the WizardComponent?

You are probably concerned by resulting code bloat. I agree, putting everything inside one class is not convenient for developers. However, I would argue that is is more convenient for users, because they have everything available right in wizard.<...>. As a user, I find it unusual that I have to reach out to wizard.model in order to, for example, get the current step.

[madoar]

No, truth be told I'm not sure whether we really need WizardState^^
I think it is ok to remove it as long as the code remains well arranged and readable.
In my opinion, better than directly adding the methods from WizardState to the WizardComponent class may be a Mixin approach. What do you think?

[earshinov]

To me, WizardState seems quite simple. I don't think that using mixins is justified here. But I'll see, maybe I will change my mind during implementation.

[earshinov]

@madoar , I have moved everything from WizardState to WizardComponent. Please review.

[earshinov]

@madoar , Thanks for review. I will get to fixing the issues in the next few days.

[madoar]

The merge conflicts need to be solved before I can merge this PR :)

[earshinov]

Now everything is fixed and ready to be merged ;-)