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

when for activation is not ng2 conform #4

Open
ribizli opened this issue May 25, 2016 · 13 comments
Open

when for activation is not ng2 conform #4

ribizli opened this issue May 25, 2016 · 13 comments
Assignees
Milestone

Comments

@ribizli
Copy link

ribizli commented May 25, 2016

It's only my opinion, but triggering via setting a variable to true for one CD iteration is dangerous and non ng2 conform.
I recommended using a template variable to trigger a notification.

<systemNotification #notification ...></systemNotification>
<button (click)="notification.notify()">show</button>

But I think an even better way would be to use a service!

@alexcastillo
Copy link
Owner

Hi @ribizli,

I like your proposal.
I'll experiment with it and get back to you.

Thank you!

@alexcastillo alexcastillo self-assigned this May 25, 2016
@alexcastillo alexcastillo added this to the v0.3.0 milestone May 25, 2016
alexcastillo added a commit that referenced this issue May 27, 2016
@alexcastillo
Copy link
Owner

@ribizli Please take a look and let me know if this is better.

@ribizli
Copy link
Author

ribizli commented May 27, 2016

Definitely, but I still think that the best solution is to use a service. There is no sense to include any component into the view if it doesn't have any view role. Sorry if this wasn't clear in my original post and you did unnecessary work.

@alexcastillo
Copy link
Owner

I already created a ticket for making a service out of this. Please refer to #9
But I still think a component is useful.

Thanks for the feedback!

@ribizli
Copy link
Author

ribizli commented May 27, 2016

One more: I guess you changed to component (from directive) because to be able to use template variable. But you can do it with directive as well using exportAs.

@alexcastillo
Copy link
Owner

Yes, I was able to make it work with exportAs but didn't like how the syntax became more complex.
Also, I changed it to a component so I could set the element to display: none;. That way it doesn't mess with the flow of the elements around it.

@ribizli
Copy link
Author

ribizli commented May 27, 2016

Still I'm curious about your argument for "component is useful" ;)

@alexcastillo
Copy link
Owner

@ribizli I agree a service can satisfy all use cases.
I think it is important to let the developer choose how to use the future. Do you think we should only have it a service and remove the component?

@ribizli
Copy link
Author

ribizli commented May 27, 2016

Yes, I see using component is more confusing for the users (developers) since it has nothing to do with the view. And if you think about usecases, a notification will come rarely from a view (by a user action), but from other service calls.

@alexcastillo
Copy link
Owner

You have a great point. Do you mind submitting a PR?

@ribizli
Copy link
Author

ribizli commented May 27, 2016

I go now for holiday for two weeks. I can't promise anything 😉

@alexcastillo
Copy link
Owner

No problem, man. I'll have something by the time you come back.

Enjoy your holiday and thank you for the support!

@LenonLopez
Copy link

LenonLopez commented Sep 23, 2017

Looking to update this project to angular 4 (5 soon enough). I would also like to implement this notification as a service rather than a component. Been a while since this repo has been updated but I would still like to contribute here rather than start from scratch. @alexcastillo thoughts?

update: As i'm doing the updates/ enhancements I realized that it is simply much easier to create a brand new project (angular 4.2.4) and port over the functionality into an injectable service. I realized after commenting that the version of angular used in this project (2.0.0-rc 1) is simply too outdated to try and manually make changes to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants