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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Disabled input for step, and prevent from moving into disabled step #177

Closed
wants to merge 1 commit into from
Closed

Add Disabled input for step, and prevent from moving into disabled step #177

wants to merge 1 commit into from

Conversation

rluvaton
Copy link

Add custom css input for navigation
Add custom CSS class for each step in navigation

Add Enums:

  • NavBar Directions
  • NavBar Layout
  • NavBar Location
  • Navigation Mode

Formatted the code

Add custom css input for navigation
Add custom CSS class for each step in navigation

Add Enums:
- NavBar Directions
- NavBar Layout
- NavBar Location
- Navigation Mode

Formatted the code
optional: isOptional(step),
navigable: isNavigable(step)
}">
[ngClass]="getStepClass(step)">
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way I can add custom class

Copy link
Owner

@madoar madoar Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of moving the json object creation to a typescript method.
But I don't understand, why a custom class needs to be added via customNavBarClass.
Starting with version 4.0.0 of angular-archwizard, the intended way to create a custom navbar indicator is via the awWizardStepSymbol directive.
For an example see https://github.com/madoar/angular-archwizard-demo/blob/develop/src/app/custom-step-symbol-template/custom-step-symbol-template.component.html.

@@ -86,16 +84,13 @@ describe('WizardComponent', () => {
expect(wizardTestFixture.debugElement.query(By.css('aw-wizard > :first-child')).name).toBe('aw-wizard-navigation-bar');
expect(wizardTestFixture.debugElement.query(By.css('aw-wizard > :last-child')).name).toBe('div');

expect(navBar.classes).toEqual({
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It no longer need to have all classes with false and true, but only the classes that needed


/**
* The layout of the navigation bar inside the wizard.
* The layout can be either small, large-filled, large-empty or large-symbols
* The layout can be either small, large-filled, large-empty, large-symbols, large-filled-symbols or large-empty-symbols
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add types of layout that didn’t written in the documentation

/**
* Navigation class
*/
private _navigationClass: any = {};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve performance instead of calculating each time update when change accord

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I like the idea to create the json object in a typescript method instead of inline in the template.
The creation of the navigationClass json object seems to be a bit complex to me.
Do we really require the ability to add a custom class here?
The intended way to change the visual design of the navigation bar is to override the navigation bar style like in https://github.com/madoar/angular-archwizard-demo/blob/develop/src/app/custom-css/custom-css.component.css.

@madoar
Copy link
Owner

madoar commented Nov 29, 2018

About the Enums:
From my personal point of view I agree with you, Enums are safer to use as input parameters compared to strings for the library.
But, in my opinion, the issue when using enums in angular is that enums are a more difficult to use as input parameters in templates compared to strings.
In addition, I haven't seen another angular library supporting enum inputs, which suggests to me that using strings is kind of standard.

Therefore I think it's better to continue using strings for the inputs.

@madoar
Copy link
Owner

madoar commented Nov 29, 2018

Just as a short note:
When providing so many unrelated changes:

  • the introduction of enums as input values
  • moving different ngClass objects to the typescript side
  • formatting changes

I believe it's better to split the different changes in individual PRs.
This makes it easier for me to comment on each distinct change and merge them accordingly.

@rluvaton
Copy link
Author

The pull request #178 contains everything that this pull request contain and more
Therefore I’m closing it

@rluvaton rluvaton closed this Nov 29, 2018
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

2 participants