appuniversum / ember-appuniversum

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

Add support for custom toaster components #419

Closed Windvis closed 1 year ago

Windvis commented 1 year ago

Builds on #418 so that needs to be merged first Done

Closes #397

TODO:

brenner-company commented 1 year ago

The funny thing is: when I was about to leave my office yesterday I saw your PR popping up and just finished a similar try-out/proof-of-concept I worked on that day 😅 I think it's pretty similar, but I've put those changes inside a draft PR if you want to give it a look: https://github.com/appuniversum/ember-appuniversum/pull/420.

I copied the usage of ensure-safe-component (used here: https://github.com/appuniversum/ember-appuniversum/pull/420/files#diff-27a3aadd521e325acf348293470dad77fa980ac290d602f2adf76f9a683f9c66R5) from here: https://github.com/mainmatter/ember-promise-modals/blob/main/addon/templates/components/modal.hbs but I'm not sure this is needed anymore? https://github.com/embroider-build/embroider/blob/main/docs/replacing-component-helper.md says it provides better support, but I'm not certain what version of Ember or types of methods we are still providing compatabilty with.

Windvis commented 1 year ago

The funny thing is: when I was about to leave my office yesterday I saw your PR popping up and just finished a similar try-out/proof-of-concept I worked on that day 😅

I assumed you hadn't gotten around to it yet and I could squeeze it in after the other toaster changes 😄. Sorry about that :no_mouth:, I should have double checked.

but I'm not sure this is needed anymore?

No, ensure-safe-component (and the component helper) shouldn't be needed here. On Ember 3.23+ Ember throws an error when users provide only a string instead of a class (this.toaster.show('some-component-name', {})). With ensure-safe-component this would work (with a deprecation warning) but I don't think we want to support that.

The component helper isn't needed either since you can simply invoke the component.

In your version that would look like this:

<toast.name @title={{toast.title}} @message={{toast.message}} @options={{toast.options}} />

I think it's pretty similar, but I've put those changes inside a draft PR if you want to give it a look: https://github.com/appuniversum/ember-appuniversum/pull/420.

If the ensure-safe-component and component helpers are removed in your PR the setup looks fairly similar. I didn't add a AuToast compontent since it didn't seem needed, unless you think it's better for projects to use AuToast over AuAlert directly?

This version also passes a @close action so that custom components can close themselves when needed.

brenner-company commented 1 year ago

I assumed you hadn't gotten around to it yet and I could squeeze it in after the other toaster changes 😄. Sorry about that 😶, I should have double checked.

That's totally ok. It can happen 😊

No, ensure-safe-component (and the component helper) shouldn't be needed here. On Ember 3.23+ Ember throws an error when users provide only a string instead of a class (this.toaster.show('some-component-name', {})). With ensure-safe-component this would work (with a deprecation warning) but I don't think we want to support that.

The component helper isn't needed either since you can simply invoke the component.

In your version that would look like this:

<toast.name @title={{toast.title}} @message={{toast.message}} @options={{toast.options}} />

That's good to know. Thanks for the extra info!

If the ensure-safe-component and component helpers are removed in your PR the setup looks fairly similar. I didn't add a AuToast compontent since it didn't seem needed, unless you think it's better for projects to use AuToast over AuAlert directly?

Not really I think. When I saw your version, my thought immediately was "Of course, it doesn't have to be that complicated".

This version also passes a @close action so that custom components can close themselves when needed.

I think that was going to my next step, but as soon as I saw your more complete version, I put mine on hold 😅

brenner-company commented 1 year ago

So the only thing left here are the test? I'd help with that, but I think you are way more familiar with setting them up within this project.

Windvis commented 1 year ago

So the only thing left here are the test? I'd help with that, but I think you are way more familiar with setting them up within this project.

Yes, I'll do it this soon. 👍

brenner-company commented 1 year ago

Looks good. Thanks for the help!