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

Stepper customization #522

Merged
merged 13 commits into from Dec 8, 2020
Merged

Stepper customization #522

merged 13 commits into from Dec 8, 2020

Conversation

joeveiga
Copy link

@joeveiga joeveiga commented Dec 2, 2020

Summary

  • Feature(Stepper): Add [large] input to increase the default icon size.
  • Feature(Stepper): Add [trackBar] input to make track bar optional.
  • Feature(Stepper): Add [progress] input to show a progress indicator on the active step.
  • Feature(Stepper): Add [removeHighlight] input to remove highlight color for the completed steps.
  • Feature(Stepper): Add [icon] input to show an icon instead of the step number.
  • Feature(Stepper): Add .complete() method to complete every step (including the last one).

image

Checklist

  • *Added unit tests
  • *Added a code reviewer
  • Added changes to /projects/swimlane/ngx-ui/CHANGELOG.md under HEAD (Unreleased)
  • Updated the demo page
  • Included screenshots of visual changes

*required

Copy link
Contributor

@nartc nartc left a comment

Choose a reason for hiding this comment

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

questions (non-blocking): LGTM but just quick question, why are there needs for using cdr.markForCheck() on every @Input?

@joeveiga
Copy link
Author

joeveiga commented Dec 3, 2020

@nartc Thanks! Good question, I'm not sure (just copy/pasted one of them xD). My best guess would be that some of those properties can be updated by code (as opposed to template binding changes). See the .first() method in the StepperComponent, for example. Maybe would be better to move the markForCheck() call out of the properties an into those methods instead?

@nartc
Copy link
Contributor

nartc commented Dec 3, 2020

suggestion (non-blocking): I see. It really depends on the intended usage of the StepperComponent. If the consumers query the StepperComponent by View/ContentChild then change StepperComponent's inputs from the consumers' component, then cdr.markForCheck might be needed. For this case, I'd try removing them. Since it looks like StepperComponent is being used with Input Bindings so StepperComppnent should be marked dirty properly without markForCheck unless there are things that I'm not aware of.

As for first(), this.active is set from inside of the StepperComponent itself so its setter should be invoked. Not sure why markForCheck() would be needed there, might be we're updating each StepComponent as well. But yeah, try removing those to see if the behavior's broken.

@joeveiga
Copy link
Author

joeveiga commented Dec 3, 2020

@nartc That particular method (first()) is used like this in the demo page:

<button class="btn" (click)="stepper.first()">First Step</button>

But yeah,, I'll try removing it and see. Thanks 🙏

@nartc
Copy link
Contributor

nartc commented Dec 4, 2020

@nartc That particular method (first()) is used like this in the demo page:

<button class="btn" (click)="stepper.first()">First Step</button>

But yeah,, I'll try removing it and see. Thanks 🙏

I missed this. Yup, in this case, cdr.markForCheck() is needed.

@Hypercubed
Copy link
Contributor

I'm not sure where this pattern came from... I would discourage using markForCheck in inputs. Most of the time it's at best redundant.

@joeveiga
Copy link
Author

joeveiga commented Dec 5, 2020

@Hypercubed I agree. I removed some of them that seemed unnecessary. However, in the case of the StepComponent, I think we do need them for the current implementation to work correctly, unless I'm missing something. Some of theStepComponent properties can be set via template binding, as in:

<ngx-stepper>
  <ngx-step [completeIcon]="myIcon"></ngx-step>
</ngx-stepper>

In which case this this._cdr.markForCheck(); call doesn't do much:

@Input()
get completeIcon() {
return this._completeIcon;
}
set completeIcon(v: string) {
this._completeIcon = v;
this._cdr.markForCheck();
}

Or they can be set as part of the containing StepperComponent:

<ngx-stepper [completeIcon]="myIcon">
  <ngx-step></ngx-step> <!-- inherits completeIcon property -->
</ngx-stepper>

And passed down to the steps using a @ContentChildren(StepComponent) reference (L112):

for (const item of this._steps.map((step, i) => ({ step, i }))) {
setTimeout(() => {
item.step.step = item.i;
item.step.active = this.active;
item.step.total = this._steps.length;
if (!item.step.completeIcon) {
item.step.completeIcon = this.completeIcon;
}
item.step.activeChange.pipe(takeUntil(this._destroy$)).subscribe(
/* istanbul ignore next */
active => (this.active = active)
);
});
}

In the second scenario, the change to the completeIcon property in the StepComponent, even though it's marked as an Input(), wouldn't be "change detected".. (or would it? 🤔 )

@nartc
Copy link
Contributor

nartc commented Dec 5, 2020

In the second scenario, the change to the completeIcon property in the StepComponent, even though it's marked as an Input(), wouldn't be "change detected".. (or would it? 🤔 )

That is correct. Angular Compiler depends on [] Input Binding syntax to actually check a component for Change Detection.
https://stackblitz.com/edit/ng-onpush-input?file=src%2Fapp%2Fhello-child.component.ts

@nartc nartc self-requested a review December 6, 2020 13:48
@joeveiga joeveiga merged commit 206af88 into master Dec 8, 2020
@joeveiga joeveiga deleted the stepper-customization branch December 8, 2020 18:35
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

4 participants