-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[API change] ConfigurableNavigationMode proposal #211
Conversation
} else { | ||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Simplify this code after "navigationForward"="visited"
is implemented.
@madoar , Please do an initial review. After that I will make changes to the demo project, if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the PR overall. I'll take a more detailed look at it later today!
export class ConfigurableNavigationMode extends BaseNavigationMode { | ||
|
||
constructor( | ||
private navigateBackward: 'allow'|'deny'|null = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we require the = null
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the line again I think I can answer that question myself: yes we do, because 'allow'|'deny'|null
doesn't allow for undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this answer is not exactly right :)
You are not using TypeScript in strict mode (not using strict
or, more specifically, strictNullChecks
compiler option in tsconfig.json
). So, in angular-archwizard's codebase, undefined
is assignable to null
. Not only that, you would also be able to assign either null
or undefined
to a variable declared as simply 'allow'|'deny'
. I have included |null
into the type signature 1) to be explicit; 2) because I am used to strict mode.
So, if we are not concerned with being strict, private navigateBackward: 'allow'|'deny'|null = null
could be alternatively declared as private navigateBackward?: 'allow'|'deny'
, the only difference being that the default value would be undefined
rather than null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About strict
mode: I've nothing about changing the angular-archwizard
code base to use strict
mode if we are able to. I'm always in favor of preventing bugs, which I think we can achieve by using strict
mode. So if you think it is a good idea to change the project to use strict
mode feel free to do so!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I welcome the idea to switch to strict
mode, but it can wait till another time.
…ault steps. See related discussion: madoar#211 (comment) This check should be restored (with "required" replaced with "non-complete") after awCompleteStep directive is implemented, see - madoar#211 (comment) - madoar#170
…gation` setter. Remove the no longer needed `onChanges` handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think the only thing that is missing is updating the README
file. I think the following points need to be changed:
README updated in 171d344 |
@madoar , FYI, I will be on vacation until the beginning of June, away from phone and Internet. Please do not expect a reply from me until 2019-06-04. |
@earshinov thanks for telling me! I wish you a nice and restful vacation! |
I've opened a new issue (#213) for the problem with my current findings. |
It makes little sense to have tests for StrictNavigationMode, SemiStrictNavigationMode, FreeNavigationMode after the corresponding classes have been removed in madoar#211
* Extract test-utils.ts with checkWizardState function * Clean up navigation unit tests It makes little sense to have tests for StrictNavigationMode, SemiStrictNavigationMode, FreeNavigationMode after the corresponding classes have been removed in #211 * Fix issues reported by Codacy, except 'magic numbers' which seem inevitable in unit tests * Remove trailing commas * Provide comments for the `checkWizardStep` function
Initial proposal in madoar/angular-archwizard-demo#31 (comment)
This PR offers a new
ConfigurableNavigationMode
class whichnavigateBackward
andnavigateForward
policies[awNavigationMode]
directive like this:Benefits of this approach:
ConfigurableNavigationMode
replaces formerStrictNavigationMode
,SemiStrictNavigationMode
,FreeNavigationMode
, making the code more compactConfigurableNavigationMode
can be extended to supportnavigationForward="visited"
to make the custom navigation mode A demo for custom navigation mode angular-archwizard-demo#31 a part of the libraryCorrespondence to the old
NavigationMode
classes:StrictNavigationMode
(default) -"navigateBackward"="allow" navigateForward="deny"
(default)SemiStrictNavigationMode
-"navigateBackward"="allow" navigateForward="allow"
FreeNavigationMode
-"navigateBackward"="allow" navigateForward="allow"
(same asSemiStrictNavigationMode
, see relevant discussion)