Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
Added :alert, :notice, and :flash as options to ActionController::Bas…
…e#redirect_to that'll automatically set the proper flash before the redirection [DHH] Added ActionController::Base#notice/= and ActionController::Base#alert/= as a convenience accessors in both the controller and the view for flash[:notice]/= and flash[:alert]/= [DHH]
- Loading branch information
Showing
6 changed files
with
98 additions
and
11 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I like the helper methods for notice/alert, I'm not sure that I'd end up using that. Most of the time that we are display flash messages, we want to make sure there are any flashes set before we attempt to render the notice/alert out.
We follow this pattern so that the HTML markup is consistent across our projects: http://github.com/planetargon/flash-message-conductor
I assume that we can still check on whether flash has anything in it?
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about flash arrays? e.g. when you have several messages that belong into the same flash key.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I use flash[:error] not flash[:alert] - naughty me.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use @response.flash[:notice] instead of @controller.send(:flash)[:notice] ?
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robby, you can keep using whatever you're doing now just fine. This pattern will allow me to extract tons of code from all my apps though.
ss, because it's a protected method.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, but I wonder if the :flash => {} option isn't a bit over the top. It doesn't really save much code or make things more readable.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition. Not only is it more succinct, I think it actually does a better job than the usual method of expressing how the flash behaves -- it's usually a bit confusing to beginners that the flash applies to the next request, and not the current request (almost no other setting in rails behaves that way). People usually seem to be surprised that working in the current request is the special case (flash.now).
Tying it in to the redirect makes the actual behavior a natural assumption.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can any light be shed on the naming of :alert versus :error ?
For some reason I've only ever seen :error (I've not seen the use of :alert until now).
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dhh, thanks :)
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great change! I always hated that it took two lines to accomplish such a simple thing.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dallas, we've been using alert for everything at 37signals. It doesn't really matter. Just that there is one accessor for go and one for stop.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh
Your AWDROR ("21.3 Flash—Communicating between Actions") uses flash[:notice], flash[:error] and flash[:warning]
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rmoriz, I'll make sure to get that updated for the next version of the book.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh
Thanks for your answer on that. I've basically learned the majority of what I know of Rails from dev teams I've been a part of and from the Agile Web Development with Rails book. :error, :alert; tom·ay·to, tom·ah·to
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why can't error and success be added?
Array has first, second, third, etc.
Go (green), Stop (red)....... how about Notice (yellow) ?
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jko, there's not enough difference to justify the extensions. If you're using :success now and want to use the built-in helpers, just switch to :notice. Or if you're married to :success, just overwrite redirect_to in your application.rb to pickup :success as well.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two don't belong in the same function, it's just more bloat. For those of you crying about having to have an "extra" line of code, are you serious? If it's that big of a deal, write a helper that will deal with your flash then redirect.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s would be nice if
automatically look at ‘users.notice.flash’ in I18n locales.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tenth ?
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea guillaumegentil !
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GuillaumeGentil really good idea,
Most of my flashes are wrapped with t() and such, behavior will really help to clean up the controllers code more
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I don't really care about the specific syntax/lexicon in this particular case, if there is going to be an established convention for something like this, it does seem like alert/notice are really very ambiguous. In fact, to me they almost seem synonymous (which isn't just not very ruby, it's not very smart). I'm sure they work fine at 37S, and they don't even seem to be causing any confusion here (we all know alert is a stop, so it's not an argument of 'which one means what?!'), but that doesn't mean they're the best option to use as the convention. Even if this is the only place these terms are hardcoded in, that's enough impetus to sway probably 90% of new projects to name their flashes this way.
I would think ten minutes of looking through a thesaurus could yield a couple words that clearly mean what they represent, which would result in most new Rails projects having meaningfully named flashes, rather than having flashes that everyone-knows-what-they-mean-even-though-it's-not-really-that-obvious. This seems like it's a no brainer; why go with option 1, which is fine but not great, when option 2 is just as easy to implement and better? If 37S is married to the crappy option, 'just overwrite redirect_to in your application.rb', no?
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally really like the convention José using in InheritedResources: :success and :failure
http://github.com/josevalim/inherited_resources
I use that one in all my projects, so I'm not that effected but this change to the Rails core - but I do agree with @farski et.al. that these flashes are a bit of "confusing naming convention". That's my 2 cents. =)
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The important thing about this commit is the combination of redirect with flash. The notice and alert helpers are mostly sugar (and they're not really synonymous with #tenth -- flash[:notice] and flash[:alert] are pretty idiomatic in the apps I've ever done).
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if you could also do this:
render :notice => 'foo'
which would set flash.now[:notice] before rendering the template.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll I upgraded from 2.3.6 to 2.3.10 and I'm nolonger able to access the notice like this ( flash[:notice] )
Now I can only access it via the controller like this ( controller.session[:flash][:notice] )
And notice does not work.
e6cadd4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very annoying that something as basic as flash is such a headache.
This is now the only way to access your flash notice? flash[:notice] and notice do not work.