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

Add flash kind for successful interactions #5969

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rmoorman
Copy link
Contributor

@rmoorman rmoorman commented Nov 8, 2024

Introduce a :success kind for flash messages in the generator output and update relevant documentation.

Success messages that previously used :info now use :success, ensuring they continue to appear in green with a "Success!" title.

In certain cases (e.g. password reset), :info is retained to avoid disclosing too much information (such as the existence of an account). These messages now appear in blue with a "Please note" title.

Rationale for those changes:

  • Differentiate informational messages from successful actions

  • Avoid confusion from informational messages styled in green, which can imply success

  • Ensure :info is used for neutral messages, and use :success to clearly indicate completed actions

@rmoorman rmoorman marked this pull request as draft November 8, 2024 16:42
@rmoorman rmoorman marked this pull request as ready for review November 8, 2024 16:45
@rmoorman rmoorman marked this pull request as draft November 8, 2024 16:47
@rmoorman rmoorman marked this pull request as ready for review November 8, 2024 17:47
@@ -46,13 +46,15 @@ defmodule <%= @web_namespace %>.CoreComponents do
role="alert"
class={[
"fixed top-2 right-2 mr-2 w-80 sm:w-96 z-50 rounded-lg p-3 ring-1",
@kind == :info && "bg-emerald-50 text-emerald-800 ring-emerald-500 fill-cyan-900",
@kind == :info && "bg-blue-50 text-blue-800 ring-blue-500 fill-blue-900",
Copy link
Contributor

Choose a reason for hiding this comment

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

personally, I would use cyan or sky for info messages, but it may be bias from seeing so many UI Frameworks using the word "info" with a light blue color

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess sky it will be then :-)

@SteffenDE
Copy link
Contributor

Hi @rmoorman,
thank you for contributing! Did you see #5844 and especially #5844 (comment)?

@rmoorman
Copy link
Contributor Author

rmoorman commented Nov 8, 2024

Oh my! @SteffenDE , I indeed didn't see that.

I suppose @josevalim didn't have a change of heart yet in this matter? 😄

IMHO it is likely that at least one neutral message kind/style is often needed (instead of green).

On one hand this could be added (like in this PR at the moment). In case it would be feasible/wanted to move this forward, of course I would also amend the inline documentation.

But on the other hand, indeed the :info style could be adjusted to be more neutral without adding another kind. I can also amend this PR to give the info flash message a more neutral styling. (wondering though how the title could be made more neutral 🤷🏻 )

Just let me know how you would like me to proceed.

Introduce a `:success` kind for flash messages in the generator
output and update relevant documentation.

Success messages that previously used `:info` now use `:success`,
ensuring they continue to appear in green with a "Success!" title.

In certain cases (e.g. password reset), `:info` is retained to avoid
disclosing too much information (such as the existence of an account).
These messages now appear in blue with a "Please note" title.

Rationale for those changes:

- Differentiate informational messages from successful actions

- Avoid confusion from informational messages styled in green, which
  can imply success

- Ensure `:info` is used for neutral messages, and use `:success` to
  clearly indicate completed actions
@glhrmv
Copy link
Contributor

glhrmv commented Nov 19, 2024

Personally I think this introduces a very useful and intuitive option that most people would likely self-implement. An "official" option makes total sense.

@SteffenDE
Copy link
Contributor

In order to move this forward, I'd say let's just change info to use sky (I visually prefer it to blue):

https://play.tailwindcss.com/Bn7JbDMpq4

image

People can easily extend their flash types themselves. If the docs aren't clear enough, we should improve those (see also phoenixframework/phoenix_live_view#3344) :)

@josevalim
Copy link
Member

@SteffenDE agreed.

@rmoorman
Copy link
Contributor Author

I will change the color to sky then.
You also seem to prefer "Info" instead of "Please note", right? I can see that it might be more in line with the single-word flash titles for the error and success cases. Also it is more grep-able and matches the status itself. I will change that as well.
And it seems there might be some conflicts too to resolve here :-)

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.

5 participants