Skip to content

Allow multiple instances of toast group#24

Closed
peaceful-james wants to merge 7 commits intosrcrip:masterfrom
peaceful-james:allow-multiple-instances-of-toast-group
Closed

Allow multiple instances of toast group#24
peaceful-james wants to merge 7 commits intosrcrip:masterfrom
peaceful-james:allow-multiple-instances-of-toast-group

Conversation

@peaceful-james
Copy link

@peaceful-james peaceful-james commented Nov 9, 2024

Just tweaks an HTML ID and adds conditional flash rendering.
This affords using toast group in live_render.
I also snuck in some cheeky data-role attributes for easier testing and CSS hacking.

<div id={assigns[:id] || "toast-group"} class={@group_class_fn.(assigns)}>
<div class="contents" id="toast-group-stream" phx-update="stream">
<div id={@id} class={@group_class_fn.(assigns)}>
<div class="contents" id={@id <> "-stream"} phx-update="stream" data-role="toast-group-stream">
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the important bit.
Without being able to change this HTML ID, we get duplicate HTML IDs when we have 2 toast groups rendered, e.g. when doing live_render

@peaceful-james
Copy link
Author

@srcrip Let me know if this PR needs anything, please.

@srcrip
Copy link
Owner

srcrip commented Dec 31, 2024

Thank you @peaceful-james! A couple thoughts:

  1. I have recently worked on something in Add delay to server-error toast #19 that added some more hard dependencies on specific IDs. Not sure how exactly we'd have to reconcile or change that PR or this PR with that in mind.
  2. This would have the impact on all flashes, not just the client and server error right? So the flag should probably be named as such? Maybe I'm wrong here.
  3. We would really need unit tests added (I just put them in the demo folder/project) around usage in live_render
  4. There was some very big changes merged recently that effect using the flash system as a proper fallback when you call put_toast immediately after a navigation. I would really like unit testing around this in particular as well, when live_render is involved

I'm not against merging this per se with some of those things addressed but I'm also curious what your use case is. Why exactly do you want to have it show up multiple times? And in your testing did this actually work properly with the state management that happens on the javascript side of the library?

@peaceful-james
Copy link
Author

I will address these comments probably no sooner than Jan 25th. Yes, it worked in my testing. Example usage: a live_render, e.g. payment form, which can be on multiple pages, which needs its own flash/feedback/toast mechanism for messages like "payment successful!".
I will tag/ping you when it is ready for re-review.

@peaceful-james peaceful-james marked this pull request as draft January 16, 2025 02:33
@srcrip
Copy link
Owner

srcrip commented Mar 21, 2025

Closing for now as it appears to be stale but feel free to reopen!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants