feat: New method changeNotification

[marxsk]

The method that simplifies editation of existing notification. In order to not duplicate a code
small refactoring was required.

[marxsk]

I believe that my patch should not impact ember-source package, so there is a potential issue in CI.

[mansona]

hey @marxsk thanks for the suggested change! I'm trying to understand the use case that you're trying to achieve here 🤔can you explain it more to me?

I'm just worried about adding complication to the addon for a bit of an esoteric use-case 🤔 I think would be happier to make sure there is a way that you could override this behaviour yourself in your local app rather than update the API, but maybe if I understood the use-case more that might be different

[marxsk]

@mansona Let's assume that you have several notifications shown at once (e.g. file downloads). Now, the file is downloaded and you want to change notification from in-progress to complete. It should stay on the same place, just the color and text is changed.

[mansona]

So firstly I kinda feel like that isn't really a use case for ember-cli-notifications 🤔 and making the api support that might make it (marginally) less useful for the intended use-case 🤔

Having said that I came across something when I was debugging an issue today: When you add a notification it gets returned from the service function

ember-cli-notifications/addon/services/notifications.js

Line 41 in 6ed526a

return notification;

Could you collect that notification and update the details in your implementation? If you do update the notification does the UI update?

[cah-brian-gantzler]

Wouldnt it be better to move the setup to the notification itself. That way you wouldnt have to call change on the service, but just call setup again on the notification itself

[mansona]

@cah-briangantzler can you elaborate? I don't quite follow 🤔

The original design is that the service has a clean API and each of the things that you can change (type, onClick callback, autoclear etc.) are just attributes on the EmberObject that is the notification. Having the API in the service that creates the notification makes sense (especially with the way that it supports defaults) and if you want to change any of the attributes on the notification you can keep track of it in a datastructure on the app side. Does that make sense?

[cah-brian-gantzler]

If you look at the change, he does intend on saving it, cause he was passing it back on the change notification. I also was wondering why he just didnt change the properties directly on the notification as you suggested. I think he through that the setup applied some logic to the initial attributes that by calling setup again he would have to reproduce, and could change several in one call, although setProperties would probably do the same thing.

Actually looking at the setup, the only logic is around auto clear. Yes, I would agree that calling setProperties on notification would do most of what he would want.

I would also advise him to use the import setProperties and not call the setProperties on the emberObject and this would allow you to rewrite the notification as a class eventually and not have to inherit from EmberObejct

[marxsk]

@mansona It is quite a while (and that code from my fork is in production for too long, in a product I no longer own) but I will try to remember :)

ad) notification is returned when it is created. IMHO, so you can track and access/identify it later. It is possible that this was my first attempt on how to change notification. So, it is possible that it is not required anymore.

ad) update - now, eveything is updated. Previously, it was not. My use-case was to show that action is in-progress and when it is finished, then it was marked as success (and auto-close was set). As some actions take dozens of seconds, I have to expect that there will be several open actions at once.

[mansona]

@marxsk ok that does make sense, thank you for clarifying!

Do you mind if we close this PR as it's quite old (as you say) and it's not a bit out of date with master? I'd be happy to chat about a potential app-level solution that would let you achieve this without needing to change the service if you ever come across it again 😄