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.

Sign up for GitHub

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

Jump to bottom

feat: Add OnPush change detection #217

Closed
wants to merge 6 commits into from
Closed

feat: Add OnPush change detection #217

wants to merge 6 commits into from

Conversation

ert78gb
Copy link
Contributor

@ert78gb ert78gb commented Sep 30, 2017

No description provided.

vlio20
vlio20 requested changes Sep 30, 2017
View reviewed changes
Copy link
Owner

@vlio20 vlio20 left a comment

Choose a reason for hiding this comment

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

In your application, what is required in order to use this component without OnPush?

src/app/day-calendar/day-calendar.component.ts Outdated
@@ -130,6 +131,7 @@ export class DayCalendarComponent implements OnInit, OnChanges, ControlValueAcce
}

writeValue(value: CalendarValue): void {
console.log('inputValue equal: ', this.inputValue === value);
Copy link
Owner

Choose a reason for hiding this comment

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

pls remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sry, removed

@ert78gb
Copy link
Contributor Author

ert78gb commented Sep 30, 2017

I will not use it without OnPush, because I use OnPush in the whole application and I don't wanna lose any performance.

@vlio20
Copy link
Owner

vlio20 commented Sep 30, 2017

The question is, is there a workaround? because currently it makes it harder to develop new features to this component.

@ert78gb
Copy link
Contributor Author

ert78gb commented Sep 30, 2017

I don't know any workaround. I don't think harder to develop. Read https://angular-2-training-book.rangle.io/handout/change-detection/change_detection_strategy_onpush.html.

@vlio20
Copy link
Owner

vlio20 commented Oct 1, 2017

I see there is a lot of over head for implementing new features. Maybe in the future when I will feel that there are no more features that I would like to add, then I will add this capability. Sorry for the time you wasted on implementing this feature - I learned from it a lot.

@vlio20 vlio20 closed this Oct 1, 2017
@ert78gb
Copy link
Contributor Author

ert78gb commented Oct 1, 2017

What kind of over head do you see?

@vlio20
Copy link
Owner

vlio20 commented Oct 1, 2017

adding cd.markForCheck();. When I am not sure that I know exactly where to put it, and even if I will know, it would be harder to others contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlio20 vlio20 vlio20 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ert78gb @vlio20