-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Correct :info flash type styling confusion #5969
base: main
Are you sure you want to change the base?
Conversation
c6c88e7
to
12c9888
Compare
Hi @rmoorman, |
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 Just let me know how you would like me to proceed. |
12c9888
to
6f4838c
Compare
Personally I think this introduces a very useful and intuitive option that most people would likely self-implement. An "official" option makes total sense. |
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 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) :) |
@SteffenDE agreed. |
I will change the color to sky then. |
464e75a
to
78529c7
Compare
Update the documentation to match the changes introduced in phoenixframework/phoenix#5969
So I went on and did the following
|
@rmoorman I think there was a misunderstanding. With my comment I wanted to say that we should keep just info and error for now, but adjust info to be less "successy", i.e., change it from green to blue. Instead of the dark blue my preference was the lighter sky blue. I do see that I didn't express this as clearly as I should have, so now I'm myself not sure if that's also what @josevalim agreed on 😅 |
@SteffenDE I am in agreement with you :) That's what I had in mind too (just change the color). |
So, you want the following adjustments @SteffenDE and/or @josevalim:
Is that correct? (P.s. I still really find it a pity because (as others mentioned as well) it seems to be a common thing to have) |
@rmoorman yes! |
I think the reasoning here for not adding success is phoenix has already been using --- @SteffenDE feel free to chime in if I have misspoken here. and I agree with this, live view currently doesn't have any strong opinions, if you want to add a success type to flashes, its relatively straight forward, and for all the generated code, you still have the ability to change it, the framework isn't stopping you in any way to use |
Correct the display of `:info` flash messages to be in line with what would be expected from an informational message (blue with "Info" in the title). Before this change, flash messages of kind `:info` would be green and have "Success!" in their title. This is wrong, as success-style feedback should only be shown on clearly successful actions and in cases neutral feedback is needed, success-style feedback does not fit. One example for this is the password reset, when we don't want to disclose whether or not a reset email has been sent. A green "Success!" flash message doesn't fit there. The other way around, a more neutral blue flash message does also fit in situations the user should receive positive feedback. In cases a distinct `:success` flash message kind is needed, it can be added in the project's core components.
78529c7
to
b52e704
Compare
@SteffenDE , I adjusted the PR to match the requested changes. It now only adjusts the styling and the title of the |
Correct the display of
:info
flash messages to be in line with whatwould be expected from an informational message (blue with "Info" in the
title).
Before this change, flash messages of kind
:info
would be green andhave "Success!" in their title. This is wrong, as success-style feedback
should only be shown on clearly successful actions and in cases neutral
feedback is needed, success-style feedback does not fit.
One example for this is the password reset, when we don't want to
disclose whether or not a reset email has been sent. A green "Success!"
flash message doesn't fit there.
The other way around, a more neutral blue flash message does also fit in
situations the user should receive positive feedback.
In cases a distinct
:success
flash message kind is needed, it can beadded in the project's core components.