goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.29k stars 137 forks source link

RFC: Toast #116

Closed goetzrobin closed 5 months ago

goetzrobin commented 8 months ago

Which scope/s are relevant/related to the feature request?

toast

Information

Current frontrunner is wrapping this great library: https://github.com/ngneat/hot-toast instead of implementing ourselves. Wondering if there are enough options to style through tw classes...

Describe any alternatives/workarounds you're currently using

No response

I would be willing to submit a PR to fix this issue

maurice0800 commented 8 months ago

Since I'd really love to use this library in one of my projects and I am missing the Toast feature, I'm willing to take a look at it!

I don't think that wrapping hot-toast is sufficient. As far as I understood it, it does not seem to support titles or action buttons inside of toasts, like shadcn or Radix do. The shadcn implementation seems to rely on putting action buttons directly into the service call like so:

toast({
    variant: "destructive",
    title: "Uh oh! Something went wrong.",
    description: "There was a problem with your request.",
    action: <ToastAction altText="Try again">Try again</ToastAction>,
})

This might be normal for React programming, but I think that we might to take a different approach because of how HTML and TS are separated in Angular.

In my opinion, a minimal API for the toast service to mimic behavior of shadcn should look like this:

toastService.open({
    title: 'Uh oh! Something went wrong.',
    description: 'There was a problem with your request.',
    variant: 'destructive' 
    action: {
        label: 'Try again',
    command: () => tryAgain(),
        altText: 'Try again',
    },
});

All parameters, except description should be optional, to allow using all different variations of toasts. Drawback of this approach would be that it only supports one action button. I don't think that this would be a problem, however we could still accept an array of actions and generate a button for each of the actions.

goetzrobin commented 8 months ago

Thanks for your feedback on this @maurice0800! We're unfortunately limited by Angular not supporting JSX the same way React does in this case.

My mind for a brain component went to allowing to use a template. I believe hot toast supports something similar. Templates can also be hidden behind Structural Directives which would allow us to expose a context from the toast.

Does that make sense? I don't love that it would separate calling the toast from its contents but that's how a flexible solution with Angular has to work. We could support passing in html though...

For the helm api surface I am wondering if something along those lines of what you're suggesting is flexible enough. (I'd lean towards yes)

maurice0800 commented 8 months ago

Yeah you are right. Somehow I totally missed the tempalte functionality of hot-toast. I think PrimeNG is using a similar approach to what you are proposing for the brain component https://primeng.org/toast#templating

I am still unsure about how we could implement this functionality. There seem to be two approaches for this:

While the hot-toast approach is more flexible, it can become annoying to specify a template every time and to share templates between many different components. The PrimeNG approach on the other hand, allows for just one type of toast per page / component, which could be limiting. Which way would you prefer?

jstnjs commented 8 months ago

Hi guys,

I was just like you @maurice0800. I also need the toast component and started working on this last week and thought: "why not implement it in Spartan as well".

My idea (and current implementation) is to port the Sonner to Angular. I've got adding toasts + adding them on several positions working already. What I still need to do is:

Plus add some more API options. For example adding a custom template. I think we could, instead of supporting jsx, support a template and even a string. That should be working fine and is pretty customizable. We could add hlm directives in the toast options to make the toaster/toasts customizable as well.

What do you guys think?

maurice0800 commented 8 months ago

@jstnjs Nice work! Could you push your changes, so we can have a look at it and discuss its current state? Maybe I could also help you with some of the tasks left

jstnjs commented 8 months ago

It's in a private repo I own. There's still lots to do, but in the end it will be the same API as the Sonner I linked. If there's interest I could probably make a branch in a fork, but need to clean some things up first :).

goetzrobin commented 8 months ago

That sounds amazing! Also happy to create a feature branch for this Sonner port 😍 Let me know what I can do to help!

jstnjs commented 8 months ago

@goetzrobin Currently just building in a repo of mine. I'll probably need a feature branch soon I guess(?). It's going pretty fast actually. Got some time on it today and added all the handlers + duration. Going to focus on adding custom template/style this week.

How do you prefer this to be added within Spartan? I was going to create a seperate library, but it might make sense to include the whole source into Spartan. What do you think?

Also, a small demo of what I currently have: https://github.com/goetzrobin/spartan/assets/33153367/45f25aa4-9a03-4e9b-81de-8fc6cc922472

goetzrobin commented 8 months ago

@goetzrobin Currently just building in a repo of mine. I'll probably need a feature branch soon I guess(?). It's going pretty fast actually. Got some time on it today and added all the handlers + duration. Going to focus on adding custom template/style this week.

How do you prefer this to be added within Spartan? I was going to create a seperate library, but it might make sense to include the whole source into Spartan. What do you think?

Also, a small demo of what I currently have:

https://github.com/goetzrobin/spartan/assets/33153367/45f25aa4-9a03-4e9b-81de-8fc6cc922472

@jstnjs That looks incredible! I'll create a toast branch tomorrow and set up the initial brain and helm libraries for you! Of course, I'd love to have this as part of spartan if you want to add it! Let me know if there's anything else I can do to support you!

jstnjs commented 7 months ago

Hi,

Small update: got the classes and styles done. Now I'm thinking about the interface for a custom template. I think got it pretty customizable right now, but I want your guys opinion.

Let's say the user wants the inside of the toast to be customizable, because the standard options such as title and description are not sufficient enough.

I've got something working which I'm pretty happy with. The user can show a toast with a service and does the call for example:

this.toasterService.message('This is by default the title', { all other options });

But what if he needs the html to be customizable? Currently this is what I have:

The user creates a new component and can add as many html/inputs as he wants:

import { Component, Input } from '@angular/core';

@Component({
  standalone: true,
  template: `{{ message }}`,
})
export class CustomToastComponent {
  @Input() message = '';
}

After creating the component, the user can call the service again, but this time with the component.

this.toasterService.warning(CustomToastComponent, {
  componentProps: {
      message: 'Text for the custom input',
  },
});

componentProps is optional if you don't have any inputs.

What do you think? I think this works pretty well.

goetzrobin commented 7 months ago

@jstnjs my mind went to a directive or a templateRef first, but I kind of like your approach too! I am wondering if we could even support both down the line? But overall I think your custom component approach is also pretty nice! Do you want me to create a feature branch for you? Also, you could always just open a PR against main, since the different primitives are independent of each other. Let me know what you prefer!

jstnjs commented 7 months ago

Probably yeah! I'll open a PR against main. Any idea on when the project will support 17.1.0? Already using input signals😄

goetzrobin commented 7 months ago

I was actually thinking about upgrading to 17.1. I was going to wait for the Nx release that adds 17.1 support, but mainly for AnalogJs to support it! The PR is already merged so once the new version is pushed to npm I'll do some dependency management!

goetzrobin commented 7 months ago

@jstnjs left this as a comment in the PR, but we now are on 17.1 on main!

goetzrobin commented 7 months ago

@jstnjs if you're already part of the Discord I can also add you to the contributors channel to facilitate communication!

goetzrobin commented 6 months ago

@jstnjs this was posted to the discord: https://discord.com/channels/1145362148621557921/1148366452102021241/1209952026201432086 Might be interesting to help get this over the finish line or connect with Clara to align the efforts? But totally up to you! I am just trying to support your efforts as good as I can!

Thanks for working on this, because this will an awesome addition 👍 I am hyped for this lol

tutkli commented 6 months ago

Hello, I'm the author of ngx-sonner. I just created an example of how would you use ngx-sonner in Spartan. Really up for debate as I'm not a fan of the wrapper component, but that's how shadcn does it.

You can check it out here: https://github.com/tutkli/spartan/tree/feature/sonner

I know you are developing your own implementation of Sonner so let me know if you want to scratch this and continue with your own.

jstnjs commented 6 months ago

@goetzrobin Oh, sorry. Totally missed your message. @tutkli Looks great! Well done. This was my first plan to do as well, but figured it would be better to build within Spartan (no dependencies, more control over the component, utilities within Spartan).

But I do realize it's been a while since I've worked on the toast component. I've been traveling a lot lately and had no focus/time to be working on this.

@goetzrobin My preference would be within Spartan, but I can see this working as well. I'm totally okay with either decision.

goetzrobin commented 6 months ago

Hey @jstnjs and @tutkli!

Looks amazing! The only reason I would like it to be part of spartan is that the repository has already found some regular contributors and users, which means we can be pretty confident that we will be able to support any added component in the long term also.

However, I don't want to compete or reinvent the wheel, because both of your work is really amazing! @tutkli, I am not sure if you'd consider adding your code to this repo and publish it as part of the spartan brain primitives, but we would of course love that!

If not, I would say we can add the dependency on your package if you are 100% bought into maintaining it for the future. Overall, I want to make sure you both know that your work is appreciated and I want to find the best way to bring it to the biggest possible audience, and selfishly use it for my own applications as fast as possible, because it's so freaking cool!

Let me know what you guys think!

jstnjs commented 6 months ago

Yeah, I was working on figuring out the brain part. How to have a unstyled toast. Probably should've focused on adding the toaster first and work out the brain part in a second version. Oh, well.

In my opinion adding the toaster in spartan feels like the best option, because then we would still have the possibility to add the brain parts. Let me know if you need my help. I'll close my PR.

tutkli commented 6 months ago

@goetzrobin Yes, I would be willing to bring my code to Spartan, however, my implementation is different than @jstnjs .

The main difference is that ngx-sonner does not have a service, instead there's a global toast() function to render the toasts. I would like to keep it this way because it respects the original code by Emil's Sonner, and also, wether we like it or not, seems like Angular is going functional in many aspects.

Spartan Sonner

Brain

This is the ngx-sonner package by itself. It has the original styles. Usage:

import { Component } from '@angular/core';
import { BrnToasterComponent, toast } from "@spartan-ng/ui-toaster-brain"

@Component {
  // ...
  standalone: true,
  imports: [BrnToasterComponent],
  template: `
    <brn-toaster />
    <button (click)="showToast()">Show toast</button>
  `
}
export class AppComponent {
  showToast() {
    toast('Hello world!');
  }
}

Helm

For styling, Shadcn uses the toastOptions input, we could use a Hlm directive to set this input, but I don't know if it would work. Thoughts?

goetzrobin commented 6 months ago

@tutkli I am cool with that! I'm curious how you achieve that internally and what made you go against a service. I think more Angular developers are used to services, but CIFs are also getting more popular and the toast function sort of looks like that!

tutkli commented 6 months ago

@goetzrobin, I know I said I'd migrate sonner to this repo, but I been looking at the implementation and I don't really like the idea.

My reasoning is that ngx-sonner has 0 dependencies, and can be a really simple notification solution for many apps. It works with css only too, you don't necesarily need tailwind. Implementing it here kinda remove the simplicity of the port.

If you don't mind, I'm going with option 2, and add the dependency. HlmToasterComponent, would be a wrapper of ngx-sonner. My initial planning was to maintain the lib for the long term and that hasn't change. I'm sorry for all the trouble,