Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Adding a helper to render all messages stored on flash object. #3009

Closed
wants to merge 1 commit into from

5 participants

@elomarns

I've added a helper to render all flash messages and customize the output if needed.

On Controller:

class PostsController < ApplicationController
  def index
    flash[:notice] = "Everything is ok :)"
    flash[:alert] =  "Error :("
  end
end

Then on the view:

<%= flash_messages %>
# => <p class="flash flash_notice">Everything is ok :)</p>
#    <p class="flash flash_alert">Error :(</p>

<%= flash_messages :with_parent_element => :div %>
# => <div class="flash flash_notice">Everything is ok :)</div>
#    <div class="flash flash_alert">Error :(</div>

<%= flash_messages :using_class => :message %>
# => <p class="message message_notice">Everything is ok :)</p>
#    <p class="message message_alert">Error :(</p>

<%= flash_messages :with_parent_element => :div, :using_class => :message %>
# => <div class="message message_notice">Everything is ok :)</div>
#    <div class="message message_alert">Error :(</div>   

<%= flash_messages :using_class => :nil %>
# => <p>Everything is ok :)</p>
#    <p>Error :(</p>
@dynaum

+1

@dmathieu
Collaborator

Nice. Though I think you should stick to the convention and use the :class => '' and :tag => '' options.

@elomarns

@dmathieu I've considered to use simpler keys like :class and :element (or :tag, as you suggested), but I've chose a more fluent interface basicaly because of two reasons:

  1. Keys like :class are frequently used on helpers that offer a html_options hash argument, but this argument generally is used to define attributes for a single element, which is not the case of flash_elements, as it can render many block elements, one for each message stored on flash.
  2. I really like this kind of interface. :)

But of course, I can change the interface if most people prefer keys like :class and :tag.

@dmathieu
Collaborator

Actually, it's a question of consistency. People will naturally use :class because that's what they can use everywhere else when calling rails helpers.

@elomarns

@dmathieu I agree with you that this helper don't have a interface consistent with other helpers. But as I see, it can't be completely consistent.

Most helpers render one HTML element, so they offer an argument to set the attributes/values of this HTML element (html_options hash argument). But on this case, the helper renders multiple HTML elements. Though html_options could be used anyway, even we're setting the attributes/values of more than one HTML element, we still need to set the parent element of each flash message. So, we would include on this hash a :tag/:element/:parent_element key, breaking the consistency of html_options, which usually only setts attributes/values, but now is setting attributes/values and the parent element.

But again, I can change the interface without problems, also because you have a definitely valid point. I just need a guideline from someone from the core team of which approach to follow, assuming they agree with this helper.

@egilburg egilburg commented on the diff
actionpack/lib/action_view/helpers/flash_helper.rb
((39 lines not shown))
+ # <%= flash_messages :using_class => :nil %>
+ # # => <p>Everything is ok :)</p>
+ # # <p>Error :(</p>
+ def flash_messages(options = {})
+ parent_element = options.delete(:with_parent_element) { :p }
+
+ messages = ""
+ options.default = "" # This is needed to use the key :using_class with nil value
+
+ flash.each do |key, value|
+ parent_element_class = class_for_flash_message_parent_element(options.merge(:flash_message_key => key))
+
+ messages << content_tag(parent_element, :class => parent_element_class) { value } + "\n"
+ end
+
+ messages.html_safe

Should this be blankly made html safe? What if your flash message reiterates user input?

I believe user input on flash messages is far from being the most common scenario. So, based on the principle of making life easier for the majority, I think people who use flash messages with content from their own code base should not have to turn the flash messages HTML safe. If the content comes from a static string from the application, I think is reasonable to suppose it's safe.

In the case you've mentioned, the flash message would have to be used without the use of this helper, which I think it's ok, as I believe this is not a common usage of flash messages, and should net harm the usage of the majority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josevalim
Owner

Thanks but this has the same problem as error_messages_for. It has too much markup in it which will lead people to provide different types of customization and the final method would become too complex while a basic version would rarely be suitable for most applications beyond the prototype stage. That said, I believe this is better left to each app.

@josevalim josevalim closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
6 actionpack/lib/action_view/helpers.rb
@@ -12,7 +12,8 @@ module Helpers #:nodoc:
autoload :ControllerHelper
autoload :CsrfHelper
autoload :DateHelper
- autoload :DebugHelper
+ autoload :DebugHelper
+ autoload :FlashHelper
autoload :FormHelper
autoload :FormOptionsHelper
autoload :FormTagHelper
@@ -42,7 +43,8 @@ module Helpers #:nodoc:
include ControllerHelper
include CsrfHelper
include DateHelper
- include DebugHelper
+ include DebugHelper
+ include FlashHelper
include FormHelper
include FormOptionsHelper
include FormTagHelper
View
69 actionpack/lib/action_view/helpers/flash_helper.rb
@@ -0,0 +1,69 @@
+module ActionView
+ module Helpers
+ module FlashHelper
+ # Helper to render all messages stored on +flash+ object. You can customize the output with the +options+ hash.
+ #
+ # ==== Options
+ # * <tt>:with_parent_element</tt> - Sets the parent element that will be used on each message (defaults to :p).
+ # * <tt>:using_class</tt> - Sets the class used on the parent element of each message (defaults to :flash).
+ #
+ # ==== Examples
+ #
+ # On Controller:
+ #
+ # class PostsController < ApplicationController
+ # def index
+ # flash[:notice] = "Everything is ok :)"
+ # flash[:alert] = "Error :("
+ # end
+ # end
+ #
+ # Then on the view:
+ #
+ # <%= flash_messages %>
+ # # => <p class="flash flash_notice">Everything is ok :)</p>
+ # # <p class="flash flash_alert">Error :(</p>
+ #
+ # <%= flash_messages :with_parent_element => :div %>
+ # # => <div class="flash flash_notice">Everything is ok :)</div>
+ # # <div class="flash flash_alert">Error :(</div>
+ #
+ # <%= flash_messages :using_class => :message %>
+ # # => <p class="message message_notice">Everything is ok :)</p>
+ # # <p class="message message_alert">Error :(</p>
+ #
+ # <%= flash_messages :with_parent_element => :div, :using_class => :message %>
+ # # => <div class="message message_notice">Everything is ok :)</div>
+ # # <div class="message message_alert">Error :(</div>
+ #
+ # <%= flash_messages :using_class => :nil %>
+ # # => <p>Everything is ok :)</p>
+ # # <p>Error :(</p>
+ def flash_messages(options = {})
+ parent_element = options.delete(:with_parent_element) { :p }
+
+ messages = ""
+ options.default = "" # This is needed to use the key :using_class with nil value
+
+ flash.each do |key, value|
+ parent_element_class = class_for_flash_message_parent_element(options.merge(:flash_message_key => key))
+
+ messages << content_tag(parent_element, :class => parent_element_class) { value } + "\n"
+ end
+
+ messages.html_safe

Should this be blankly made html safe? What if your flash message reiterates user input?

I believe user input on flash messages is far from being the most common scenario. So, based on the principle of making life easier for the majority, I think people who use flash messages with content from their own code base should not have to turn the flash messages HTML safe. If the content comes from a static string from the application, I think is reasonable to suppose it's safe.

In the case you've mentioned, the flash message would have to be used without the use of this helper, which I think it's ok, as I believe this is not a common usage of flash messages, and should net harm the usage of the majority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
+ protected
+ def class_for_flash_message_parent_element(options = {})
+ if options[:using_class]
+ parent_element_class = (options.delete(:using_class) { "flash" }).to_s
+
+ "#{parent_element_class} #{parent_element_class}_#{options[:flash_message_key]}"
+ else
+ nil
+ end
+ end
+ end
+ end
+end
View
62 actionpack/test/template/flash_helper_test.rb
@@ -0,0 +1,62 @@
+# encoding: utf-8
+
+require 'abstract_unit'
+
+class FlashHelperTest < ActionView::TestCase
+ tests ActionView::Helpers::FlashHelper
+
+ def test_flash_messages_without_messages
+ assert flash_messages.blank?
+ end
+
+ def test_flash_messages_is_html_safe
+ flash[:notice] = "Some random resource was succesfuly saved."
+
+ assert flash_messages.html_safe?
+ end
+
+ def test_flash_messages_with_notice_message
+ flash[:notice] = "Everything is ok."
+
+ assert_equal %Q(<p class="flash flash_notice">Everything is ok.</p>\n), flash_messages
+ end
+
+ def test_flash_messages_with_alert_message
+ flash[:alert] = "10 minutes to world explosion."
+
+ assert_equal %Q(<p class="flash flash_alert">10 minutes to world explosion.</p>\n), flash_messages
+ end
+
+ def test_flash_messages_with_notice_and_alert_messages
+ flash[:notice] = "This application is just fine."
+ flash[:alert] = "The previous message is wrong!"
+
+ messages = flash_messages
+
+ assert messages.include?(%Q(<p class="flash flash_notice">This application is just fine.</p>\n))
+ messages.gsub!(%Q(<p class="flash flash_notice">This application is just fine.</p>\n), "")
+
+ assert messages.include?(%Q(<p class="flash flash_alert">The previous message is wrong!</p>\n))
+ messages.gsub!(%Q(<p class="flash flash_alert">The previous message is wrong!</p>\n), "")
+
+ assert messages.blank?
+ end
+
+ def test_flash_messages_with_custom_parent_element
+ flash[:notice] = "Look at me mom, I'm on a flash message!"
+
+ assert_equal %Q(<div class="flash flash_notice">Look at me mom, I'm on a flash message!</div>\n), flash_messages(:with_parent_element => :div)
+ end
+
+ def test_flash_messages_with_custom_class_for_parent_element
+ flash[:alert] = "Houston, we have a problem."
+
+ assert_equal %Q(<p class="message message_alert">Houston, we have a problem.</p>\n), flash_messages(:using_class => :message)
+ end
+
+ def test_flash_messages_without_class_on_parent_element
+ flash[:notice] = "Up and running!"
+
+ assert_equal %Q(<p>Up and running!</p>\n), flash_messages(:using_class => nil)
+ end
+end
Something went wrong with that request. Please try again.