Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple instances of toast group #24

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

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
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