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 step change and disabled step option #178

Closed
wants to merge 2 commits into from
Closed

Add step change and disabled step option #178

wants to merge 2 commits into from

Conversation

rluvaton
Copy link

Add step change event to wizard #176
Fix wizard component test file misleading comment

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
Fix wizard component test file misleading comment
@rluvaton
Copy link
Author

In case of approval need to update the README file of the new changes

@rluvaton rluvaton changed the title Add step change Add step change add disabled step option Nov 29, 2018
@rluvaton rluvaton changed the title Add step change add disabled step option Add step change and disabled step option Nov 29, 2018
* Step Changed event that fired with the new current step
*/
@Output()
stepChanged: EventEmitter<WizardStep> = new EventEmitter<WizardStep>();
Copy link
Owner

Choose a reason for hiding this comment

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

I believe it's a good idea to add a stepChanged output emitter.
I'm just not sure if we should emit the WizardStep object, to which the wizard changed.
The reason here is, that the WizardStep is meant as an wizard internal class, which provides a lot of fields which shouldn't be changed outside of the wizard (for example the field selected shouldn't be changed from outside the wizard).

In addition the output doesn't forward information such as the navigation direction, the source step etc, which is often also of interest.
I think it's better to brainstorm some more about which information the output should forward and how it should look like.

Copy link
Author

Choose a reason for hiding this comment

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

You’re absolutely right,
What about this?

{
  from: number,
  to: number,
  direction: MovingDirection
}

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good.
Are there maybe some other fields we want to pass to the receiver of the output emitter (just asking would be bad if we missed something)?

Copy link
Author

Choose a reason for hiding this comment

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

I don’t think it’s necessary but you can provide source and isTrusted:

Source

source of the changed step: from where, is it from clicking next/prev/go-to step or by clicking the step title

isTrusted

Is the change came from our library:

  • Next/Prev/Go To step directives
  • step title
  • or programmatically

this._currentStepIndex = index;

if (this._currentStepIndex >= 0) {
this.stepChanged.next(this.wizardSteps[this._currentStepIndex]);
Copy link
Owner

Choose a reason for hiding this comment

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

This should be moved to the navigation modes

Copy link
Author

Choose a reason for hiding this comment

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

I thought of that at first but then for each type of navigation mode (strict and etc) need to be the same code, so why don’t we make it once in a service that have current index already

Copy link
Owner

Choose a reason for hiding this comment

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

This will change with #166, where this redundant code is likely to be moved to BaseNavigationMode.
We just need to wait until @earshinov has some free time again to continue working on the PR.

/**
* Step Changed Subject for notify and update when step changed
*/
private stepChanged: Subject<WizardStep> = new Subject<WizardStep>();
Copy link
Owner

Choose a reason for hiding this comment

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

Why is an additional Subject required?
Isn't the output sufficient?

Copy link
Author

Choose a reason for hiding this comment

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

I don’t think you can use Output in an Angular service

Copy link
Owner

Choose a reason for hiding this comment

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

How do you access the Subject from the angular service?
Hasn't it the same issue?

Copy link
Author

Choose a reason for hiding this comment

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

You don’t you have limited options in purpose.

You only need 2 usages with this

  1. Update step
  2. Listen to updates

Update step:
When changing current index it will emit the value

Listen to changes:
You have a get variable/function for getting the observable

Copy link
Owner

Choose a reason for hiding this comment

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

Can't you then create the Subject in your service and wire the emitted step change to the subject by a provided function to the output?
I.e. like this:

stepChanged(event): void {
   this.service.subject.next(event);
}

<wizard (stepChanged)="stepChanged($event)">
   ...
</wizard>

Copy link
Author

Choose a reason for hiding this comment

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

Where is the step changed function location? is it in the wizard.component.ts because you can create instance of yourself

Is the user have this function? Then we don’t provide him the subject only update and listening, can you please give me another example?

Copy link
Author

@rluvaton rluvaton Nov 29, 2018

Choose a reason for hiding this comment

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

And I think using an angular built in component tool is better for understanding and simpler

Copy link
Owner

@madoar madoar Nov 30, 2018

Choose a reason for hiding this comment

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

Here a more detailed example:

The service

@Injectable()
export class Service {
   public subject = new Subject();
}

The application component

@Component({
   template: `
      <wizard (stepChanged)="stepChanged($event)">
         ...
      </wizard>
   `
})
export class AppComponent {
   constructor(private service: Service) {}

   public stepChanged(event): void {
      this.service.subject.next(event);
   }
}

Copy link
Owner

@madoar madoar Nov 30, 2018

Choose a reason for hiding this comment

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

I agree, that adding functions directly to a library is often faster to implement and second easier to use from inside an end-application.

The issues for the library developers are, that each additional function needs to be documented and maintained.
In addition this easily leads to cluttering of the library, where a lot of functions are added to a library that are not often needed or which can be easily realized from outside of the library.

Copy link
Author

Choose a reason for hiding this comment

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

So at least make the subject private and leave the functions for interact with the subject without changing the subject itself, this way you don’t open the subject for unwanted changes

/**
* A boolean describing if the wizard step is an disabled step
*/
public disabled = false;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand what a disabling a step does?

Copy link
Author

Choose a reason for hiding this comment

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

Preventing you from entering this step

Use case:

  • you need to choose between 2 options each option make different step enabled

I needed to use this option actually

Copy link
Owner

Choose a reason for hiding this comment

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

This sounds like you want to use the inputs [canEnter] and [canExit] of the wizard steps?

Copy link
Author

Choose a reason for hiding this comment

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

Not precisely, because disabled is a state, it’s need to have a style and custom behavior:

Like when going to next step it will jump over the disabled one.

And other libraries have this too...

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I understand.
Then the questions are:

  • how do you define a step as disabled?
  • how should the wizard react when the user wants to navigate to a disabled step?
    Is it always possible to skip the step? If yes, in which direction? Are there other actions the wizard could do in instead of entering a disabled step?

In any case I think supporting disabled steps should be moved to its own PR.

@madoar
Copy link
Owner

madoar commented May 15, 2019

Closed because of inactivity

@madoar madoar closed this May 15, 2019
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