Refactor the hierarchy of NavigationMode classes.

[earshinov]

Shared code extracted to the base abstract class NavigationMode.

This highlights differences between provided navigation modes.

Let us continue the refactoring marathon ;-)

[earshinov]

Actually, I would also like to share a custom navigation mode we implemented in our project, in case you find it useful enough to be included in the library. However, I'd like this refactoring to be handled first, because (apart from other benefits it brings) it will make it easier for me to point out the differences between our custom nav. mode and the built-in ones.

[madoar]

I like the refactoring of the navigation modes.
In the following I've added some additional thoughts of mine:

NavigationMode interface

What I'm not sure about is, whether we should really remove the NavigationMode interface, or whether we should just add a new abstract class, which we add in-between, i.e. an abstract class which implements the NavigationMode interface and is extended by all current concrete navigation mode implementations.
This should allow for a more free implementation of new navigation modes.

Injectability

Currently the navigation mode is created by the wizard itself (see https://github.com/madoar/angular-archwizard/blob/develop/src/lib/navigation/wizard-state.model.ts#L107).
Ideally I would like to change this to an injection based creation, i.e. angular itself creates the navigation mode based in the input to wizard.

Comment style

The current comment style is based on typedoc (if I remember correctly).
One of my plans for the library is to publish an API documentation for the wizard on the corresponding GitHub page.
For this I want to change the documentation style to compodoc, therefore it would be useful to modify the comment style during the refactoring to be compatible with the new documentation tool.

New navigation mode

I'm always open for new code additions like navigation modes!
The only important point is, that they shouldn't target a too limited audience, because otherwise I fear that too many such changes reduce the maintainability of the wizard code.

[earshinov]

Regarding comment style, I am not sure the project can be migrated to compodoc because compodoc does not support some useful constructs used in the project (namely: @author, @throws).

[earshinov]

As for separating a base abstract class and an interface, free implementation isn't a good purpose, imo. That is, if I had to implement my own navigation mode, I cannot imagine circumstances when I would choose the interface over the abstract class, because I would have to copy a lot of code (goToStep is good example) from the abstract class in order for the wizard to work correctly.

However, there might by other purposes of extracting an interface. For example, maybe it could serve as a marker for Angular Injector (going to look through the Injectability section now).

[earshinov]

Now, the Injectability section. I see one problem with this: each wizard has to have its own instance of NavigationMode, which, in Angular, means that NavigationMode has to be provided in the WizardComponent (as long as we support having multiple wizard on a page, but I believe we do). So, that's the end of the way.

Or we can inject a factory function, but we still need to provide a way to pass such a factory as an input to preserve an ability to set up several wizards on the same page differently. Existing navigationMode input is a good candidate, we just need to extend it:

Previous signature: string
New signature: string|((wizard: WizardComponent) => (string|NavigationMode)

This way it will be possible to either construct a brand new NavigationMode or opt for a built-in navigation mode by returning its special name (like "semi-strict").

We also can provide an injection token which user could provide for setting default configuration for all affected angular-archwizard's, like, for example, in ngx-perfect-scrollbar. If a user wants all wizard to have the same navigation mode, he/she will be able to set it in a single place.

@NgModule({
  providers: [
    // ...
    { provide: ANGULAR_ARCHWIZARD_CONFIG, useValue: { navigationMode: "semi-strict" } }
  ]
})
export class AppModule { }

Does it make sense?

[madoar]

About the interface vs abstract class topic:
An interface in general has the benefit, that it provides the developer with complete freedom about how he wants to provide his implementation.
This isn't possible with an abstract class, that provides some predefined method implementations, these methods will sometimes limit the freedom of the developer in creating his implementation.

I'm not a friend of injection tokens/markers for interfaces.
I would prefer to use an abstract class as the marker, like I did here.
I don't meant to say that I absolutely require an interface at the top-most level in the navigation mode implementation hierarchy, I just think it is useful to have from an implementation point of view.

[madoar]

About the Injectability of the navigation mode:
Yes, each wizard needs its own navigation mode instance.
This would require, that the navigation mode is provided for each WizardComponent.
I'm not sure this is really a problem, because the same applies to the wizard state, which is provided at the WizardComponent level.

I like the idea to extend the navigationMode input to the signature string|((wizard: WizardComponent) => (string|NavigationMode)).
This would make the wizard much more flexible.

About the injection token usage for the selection of a navigation mode:
It is hard for me to measure the benefits of this approach, compared to the changed signature approach.

[earshinov]

As an option, we could exclude this method from the interface, but in this case existing tests will have to be rewritten to test BaseNavigationMode's instead of NavigationMode's to be able to still call canGoToStep. @madoar , what is your opinion on this?

[madoar]

I kind of like the idea to move canGoToStep from the interface to the base class, but I see a possible issue with this:

canGoToStep is used to determine if an arbitrary given step can be entered.
This is also the main difference in my opinion between canGoToStep and isNavigable, because isNavigable will always returns false for a future step in the strict navigation mode. The method can therefore be used by an application developer to query the wizard if an arbitrary step can be entered.
Such a query mechanism isn't provided by another method of the interface, because the "main" methods, i.e. the goTo methods don't provide a return value.
I'm not sure if we should remove this method without providing a replacement method to do the same.

[earshinov]

OK, let's leave this method intact.

[earshinov]

To make the interface cleaner, we could exclude goToPrevious/NextStep methods from it, but users will then have to call goToStep(wizardState.destinationIndex ± 1), which is inconvenient. Also, existing tests will have to be adapted. It feels bad either way. For now, I would choose having these methods in the interface. @madoar, your opinion?

[madoar]

Every time I need to touch the interface/implementation classes I ask myself the same question^^
The big benefit of having the methods is the additional syntactic difference between + and - 1, i.e. got the next or previous step.
I think this additional safety measurement helps to prevent the introduction of bugs, so I previously decided to let the methods remain inside the interface.

If you have a better idea to preventing such bugs or make it harder for them to occur, I'm free with removing the methods.

[earshinov]

No, I can't think of a better idea now. Let's leave the goToPrevious/NextStep methods intact.

[earshinov]

About the injection token usage for the selection of a navigation mode:
It is hard for me to measure the benefits of this approach, compared to the changed signature approach.

I think we've got our wires crossed here :) I did not mean to use this approach inside the library. Instead, I meant that we could provide such option to the user for the purpose of global customization of aw. The navigationModeFactory could be turned into an interface, which a user could provide in order to, for example:

• globally select a default navigation mode other than 'strict';
• support user's own string names for navigation mode (an illustration follows).

Illustration:

// AppModule
providers: [
  { provide: WizardNavigationModeFactory, useClass: AppWizardNavigationModeFactory },
]

// wizard-navigation-mode-factory.service.ts
//export class AppWizardNavigationModeFactory implements WizardNavigationModeFactory {
export class AppWizardNavigationModeFactory extends WizardBaseNavigationModeFactory {
  public createByName(wizard: WizardComponent, name: string) {
    if (name == 'custom') {
      // support custom navigation mode
      return new CustomNavigationMode(wizard.model);
    }
   // support standard navigation modes like 'strict' etc.
   return super().createByName(wizard, name);
  }
}

@madoar, do you consider this useful and worth implementing?

[madoar]

I like it!
Especially the factory class approach compared to a only factory method approach.
When implementing this, it's important that we have a test for it and I think, that the following case is supported to:

export class AppWizardNavigationModeFactory extends WizardBaseNavigationModeFactory {
  public createByName(wizard: WizardComponent) {
    // always return the custom implementation independent of the input parameter
    return new CustomNavigationMode(wizard.model);
  }
}

Alternatively the WizardBaseNavigationModeFactory could provide multiple methods a later custom factory could override, for example:

public createDefault(wizard: WizardComponent);
public createDefault(model: WizardState);
public createByName(wizard: WizardComponent, name: String);
...

[earshinov]

@madoar , I apologize again for the delays, it is crunch time at work :-( Will probably be able to return to archwizard this weekend.

[madoar]

No problem :)

[madoar]

@earshinov I hope you're still fine?
If you need some help with this feel free to tell me, it would be nice to see this merged :)

[earshinov]

@madoar , Hi! I am still a bit overworked, but I have reason to believe than I will be able return to archwizard in the middle of the upcoming week.

[madoar]

@earshinov if you think this is ready to be merged I'll take a look at this PR the coming weekend.
I let this PR remain open until now because of your last suggestion and my following comment #166 (comment):

I think we've got our wires crossed here :) I did not mean to use this approach inside the library. Instead, I meant that we could provide such option to the user for the purpose of global customization of aw. The navigationModeFactory could be turned into an interface, which a user could provide in order to, for example:

• globally select a default navigation mode other than 'strict';
• support user's own string names for navigation mode (an illustration follows).

[earshinov]

@madoar , No, I don't think it is ready to be merged yet. I would like to first review our discussion and your comments, probably this weekend.

[earshinov]

Hello @madoar , I have extracted NavigationModeFactory interface and BaseNavigationModeFactory class as we planner earlier. Awaiting your comments.

[earshinov]

I have also created a new demo page in the angular-archwizard-demo project, see madoar/angular-archwizard-demo#31

[madoar]

config && config.navigationModeFactory allows the library user to overwrite the default factory (i.e. BaseNavigationModeFactory), right?

[earshinov]

Yes, this construction makes sure that user's setting only takes effect when config is not falsy (defined and not null) and config.navigationModeFactory is not falsy (defined and not null). Otherwise BaseNavigationModeFactory will be used.

This expression can be rewritten as:

config && config.navigationModeFactory ? config.navigationModeFactory : BaseNavigationModeFactory

which is a little bit longer.