appuniversum / ember-appuniversum

Ember addon wrapping the appuniversum components.
https://appuniversum.github.io/ember-appuniversum
MIT License
14 stars 10 forks source link

Addition of action button within toaster (& alert) component #401

Closed brenner-company closed 1 year ago

brenner-company commented 1 year ago

Addition of the following functionality for the AuToaster & AuAlert components:

What is currently missing (within draft):

PS: @Windvis, I've checked your comment at https://github.com/appuniversum/ember-appuniversum/issues/397#issuecomment-1621658212. That seems like a great method to implement similar changes, but I don't think this has much advantage over the current version/changes within this draft? Feel free to correct me if this is not the case!

Windvis commented 1 year ago

PS: @Windvis, I've checked your comment at https://github.com/appuniversum/ember-appuniversum/issues/397#issuecomment-1621658212. That seems like a great method to implement similar changes, but I don't think this has much advantage over the current version/changes within this draft? Feel free to correct me if this is not the case!

The main reason I preferred the flexible version is that every project can then do what they want. I'm not sure that 1 action button covers all the use cases. It probably does, but when it doesn't we will have to do some weird API changes to make it possible. With the component version we still had that as a fallback.

I can see why a stricter API is a good thing though. All projects would do it in a similar way and it's easier to use.

I looked at the Webuniversum version again and it seems they do actions slightly different there: https://overheid.vlaanderen.be/webuniversum/v3/documentation/js-components/vl-ui-toaster (although they do only show 1 action button).

Maybe we should try to align the design?

brenner-company commented 1 year ago

The main reason I preferred the flexible version is that every project can then do what they want. I'm not sure that 1 action button covers all the use cases. It probably does, but when it doesn't we will have to do some weird API changes to make it possible. With the component version we still had that as a fallback.

I can see why a stricter API is a good thing though. All projects would do it in a similar way and it's easier to use.

I can give this a look tomorrow and create a version within a new branch. But I'm not sure how much time I can spend on this because this effort is getting more and more in the ember-appuniversum (as this is becoming a bit of a refactor?) and not Kaleidos territory. I'll keep you posted.

Functionality wise, if I understand you correctly I can rework addon/services/toaster.js a bit so we still have the .notify(), .success(), etc. functions but with a new .show() one? Or do we remove those and use .show() altogether?

And do we add the new custom component (toast with action button) to ember-appuniversum itself? Together with the alignment with the design of course.

I looked at the Webuniversum version again and it seems they do actions slightly different there: https://overheid.vlaanderen.be/webuniversum/v3/documentation/js-components/vl-ui-toaster (although they do only show 1 action button).

Maybe we should try to align the design?

That is no a bad idea I guess? So, if the toaster needs to be the warning version/skin with an action button, that would be an exception to the rule and would be the perfect case for a custom toaster component within the destination project?

Windvis commented 1 year ago

I can give this a look tomorrow and create a version within a new branch.

No, no need, keep it like this. We can always add the component version in the future and deprecate the action options if needed.

But I'm not sure how much time I can spend on this because this effort is getting more and more in the ember-appuniversum (as this is becoming a bit of a refactor?) and not Kaleidos territory. I'll keep you posted.

Well, this is the hard part for me. My personal opinion is that Appuniversum should be as generic as possible and features should only be added if it will be widely used.

To me it was not clear if this "action in a notification" is a generic feature or something only Kaleidos needed. So my preference was a generic / lower level solution which apps could build on, without having to ship the opinionated version as part of Appuniversum itself. But it seems there are examples of this case in Webuniversum, so that seems good enough reason to ship the higher level implementation instead.

Windvis commented 1 year ago

(let me know if this is blocked by me in some way)

brenner-company commented 1 year ago

I think I'll need to extend this PR a tad more with additional functionality regarding the button skin, but I'll pick that up when I get back.

Thanks for the follow-up!

brenner-company commented 1 year ago

I've just heard that we'll also need a AuLinkExternal inside another variant of the toast, so I'm afraid the current version of the PR won't be sufficient anymore.

Is ok for you if I give the component version (mentioned here: https://github.com/appuniversum/ember-appuniversum/issues/397#issuecomment-1621658212) a look tomorrow?

Windvis commented 1 year ago

I have some time this sprint, so I gave it a shot in https://github.com/appuniversum/ember-appuniversum/pull/419

Needs some finetuning, but seems to work 👍.