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

handle_params, push_patch & put_toast #9

Open
tomeq93 opened this issue Jun 15, 2024 · 8 comments
Open

handle_params, push_patch & put_toast #9

tomeq93 opened this issue Jun 15, 2024 · 8 comments

Comments

@tomeq93
Copy link

tomeq93 commented Jun 15, 2024

Hey,
If I use put_toast with combination of push_patch in handle_params I do not have toast visible and it works with default put_flash, any ideas? Thanks, great lib btw :)

@srcrip
Copy link
Owner

srcrip commented Jul 3, 2024

Interesting... I will try to replicate but if there's any way you can provide a minimal repro it would help a lot. Thanks!

@tomeq93
Copy link
Author

tomeq93 commented Jul 3, 2024

tbh. I can't reproduce it on newest version {:phoenix_live_view, "~> 1.0.0-rc.6", override: true},

¯\ (ツ)

@tomeq93 tomeq93 closed this as completed Jul 3, 2024
@tomeq93
Copy link
Author

tomeq93 commented Jul 3, 2024

ahh forgot it was from handle_params -> git patch with repro

From bd35de5be3994f96786a8f4266a1697405871e1b Mon Sep 17 00:00:00 2001
From: example <[email protected]>
Date: Wed, 3 Jul 2024 15:04:32 +0200
Subject: [PATCH] repro

---
 demo/lib/demo_web/live/home_live.ex | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/demo/lib/demo_web/live/home_live.ex b/demo/lib/demo_web/live/home_live.ex
index 8a0639c..ebda584 100644
--- a/demo/lib/demo_web/live/home_live.ex
+++ b/demo/lib/demo_web/live/home_live.ex
@@ -14,7 +14,7 @@ defmodule DemoWeb.HomeLive do
     "action" => nil
   }
 
-  def handle_params(_params, _uri, socket) do
+  def handle_params(params, _uri, socket) do
     socket =
       socket
       |> assign(:settings, @default_settings)
@@ -23,7 +23,15 @@ defmodule DemoWeb.HomeLive do
 
     Logger.info("Mounted")
 
-    {:noreply, socket}
+    if params["test"] do
+      {:noreply,
+       socket
+        |> put_flash(:info, "I'm working")
+      #  |> LiveToast.put_toast(:error, "I'm not")
+       |> push_patch(to: "/")}
+    else
+      {:noreply, socket}
+    end
   end
 
   def handle_event("change_settings", payload, socket) do
-- 
2.45.2

@tomeq93 tomeq93 reopened this Jul 3, 2024
@srcrip
Copy link
Owner

srcrip commented Jul 6, 2024

I have a plan for how to fix this. It should be ready in a few days.

@srcrip
Copy link
Owner

srcrip commented Jul 9, 2024

So I have a branch that fully 'fixes' this issue. It is however quite complicated. It requires basically, on every call to put_toast, pushing both a flash and a toast, and deduping by deleting the flash in the Live Component if it receives an update that contains both a flash and toast of the same kind and message.

That's a little janky, but testable and verifiable I guess. The bigger thing is it also requires passing said toast synchronously through params to the LC. That is a pretty big change that requires a lot to make work. It also means that the param most likely needs to be set to a temporary_assign. That's kind of a problem, cause whatever LV is mounted to the router would have to be responsible for that. We could however make a __using__ macro that does this and just expect the user to have to put it in there web module LV callback so it fires for every LV.

There is another alternative which is pretty simple: we can also just check the socket for the presence of the redirected attribute in put_toast, and if it's there, put a flash on instead of a toast. This works great but now the order that you call the functions matter. You'd have to call push_navigate before put_toast. It has the advantage of being an extremely simple fix though.

The final option is to just say well, if you want messages to work across navigates just use put_flash That's an option, but I don't know if I'm fully satisfied with it.

Like I said, I have a branch that kind of makes it all 'just work', but it's complicated. I think I need to test it more myself and make sure it doesn't add some crazy levels of jank. I can even push it up for others to try at some point soon. If it's not a suitable fix though I think I'll lean towards option two, which is literally a one line change and just requires you to change your user-space code a little bit.

@tomeq93
Copy link
Author

tomeq93 commented Jul 10, 2024

Thanks for detailed response, I prefer using put_flash in this cases explicitly in my codebase (would be nice to have readme description to not be surprised) instead of library doing this under the hood, regarding complicated logic maybe you could ping on elixirforum if it's the right / only way 🤔

@sergchernata
Copy link

Can we just auto-dismiss the flash after the redirect, after some number of seconds?

@MarkoH17
Copy link

MarkoH17 commented Dec 5, 2024

@srcrip Is there any update on this? I didn't see the branch with the experimental fixes in this repo.

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

No branches or pull requests

4 participants