Consolidate flash[:some_name] messages #19

Closed
benjaminvialle opened this Issue Sep 29, 2010 · 11 comments

Comments

Projects
None yet
3 participants
@benjaminvialle
Member

benjaminvialle commented Sep 29, 2010

At various places in our controllers different flash[] notices are used. For example, there are at least

* flash[:fail_notice]
* flash[:upload_notice]
* flash[:invalid_lines]
* flash[:users_not_found]
* flash[:edit_notice]

I think it would be beneficial if we could come up with a sort of standard set, which we document on the wiki as to how what to use when and how. Have a short description of each and what it does, etc.

This is especially useful for UI refactoring. Assume MarkUs will be undertaken, yet another UI redesign. The person in charge of the UI would know that the documented set has to be implemented. What's more, developers have a place to look at for controller development.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 21, 2013

Member

I found the following:

flash[:error]
flash[:success]
flash[:annotation_upload_invalid_lines]
flash[:annotation_upload_success]
flash[:fail_notice]
flash[:edit_notice]
flash[:invalid_lines]
flash[:upload_notice]
flash[:login_notice]
flash[:note_success]
flash[:fail_notice]
flash[:file_download_error]
flash[:transaction_warning]
flash[:users_not_found]
flash[:file_download_error]
flash[:update_conflicts]
flash[:commit_notice]
flash[:release_results]
flash[:commit_error]
flash[:release_errors]

Perhaps the following would work instead?

flash[:error]
flash[:success]
flash[:warning]
flash[:notice]

They could map as such:

flash[:error]                               => flash[:error]
flash[:success]                             => flash[:success]
flash[:annotation_upload_invalid_lines]     => flash[:error]
flash[:annotation_upload_success]           => flash[:success]
flash[:fail_notice]                         => flash[:notice]
flash[:edit_notice]                         => flash[:notice]
flash[:invalid_lines]                       => flash[:error]
flash[:upload_notice]                       => flash[:notice]
flash[:login_notice]                        => flash[:notice]
flash[:note_success]                        => flash[:success]
flash[:fail_notice]                         => flash[:notice]
flash[:file_download_error]                 => flash[:error]
flash[:transaction_warning]                 => flash[:warning]
flash[:users_not_found]                     => flash[:error]
flash[:file_download_error]                 => flash[:error]
flash[:update_conflicts]                    => flash[:warning]
flash[:commit_notice]                       => flash[:notice]
flash[:release_results]                     => flash[:success]
flash[:commit_error]                        => flash[:error]
flash[:release_errors]                      => flash[:error]

Any thoughts on this? With a bit of feedback, I can tackle this cleanup.

Member

danielstjules commented Jan 21, 2013

I found the following:

flash[:error]
flash[:success]
flash[:annotation_upload_invalid_lines]
flash[:annotation_upload_success]
flash[:fail_notice]
flash[:edit_notice]
flash[:invalid_lines]
flash[:upload_notice]
flash[:login_notice]
flash[:note_success]
flash[:fail_notice]
flash[:file_download_error]
flash[:transaction_warning]
flash[:users_not_found]
flash[:file_download_error]
flash[:update_conflicts]
flash[:commit_notice]
flash[:release_results]
flash[:commit_error]
flash[:release_errors]

Perhaps the following would work instead?

flash[:error]
flash[:success]
flash[:warning]
flash[:notice]

They could map as such:

flash[:error]                               => flash[:error]
flash[:success]                             => flash[:success]
flash[:annotation_upload_invalid_lines]     => flash[:error]
flash[:annotation_upload_success]           => flash[:success]
flash[:fail_notice]                         => flash[:notice]
flash[:edit_notice]                         => flash[:notice]
flash[:invalid_lines]                       => flash[:error]
flash[:upload_notice]                       => flash[:notice]
flash[:login_notice]                        => flash[:notice]
flash[:note_success]                        => flash[:success]
flash[:fail_notice]                         => flash[:notice]
flash[:file_download_error]                 => flash[:error]
flash[:transaction_warning]                 => flash[:warning]
flash[:users_not_found]                     => flash[:error]
flash[:file_download_error]                 => flash[:error]
flash[:update_conflicts]                    => flash[:warning]
flash[:commit_notice]                       => flash[:notice]
flash[:release_results]                     => flash[:success]
flash[:commit_error]                        => flash[:error]
flash[:release_errors]                      => flash[:error]

Any thoughts on this? With a bit of feedback, I can tackle this cleanup.

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Jan 21, 2013

Member

Yes, your proposal looks fine.

+1 for a go.

Maybe @NelleV could tell us if it looks fine, as our UI expert ;)

Member

benjaminvialle commented Jan 21, 2013

Yes, your proposal looks fine.

+1 for a go.

Maybe @NelleV could tell us if it looks fine, as our UI expert ;)

@NelleV

This comment has been minimized.

Show comment
Hide comment
@NelleV

NelleV Jan 21, 2013

Member

LGTM

Member

NelleV commented Jan 21, 2013

LGTM

@ghost ghost assigned danielstjules Jan 21, 2013

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Jan 21, 2013

Member

@danielstjules I assigned you the issue.

Member

benjaminvialle commented Jan 21, 2013

@danielstjules I assigned you the issue.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 21, 2013

Member

Thank you, I'll work on it once I correct my current pull.

Member

danielstjules commented Jan 21, 2013

Thank you, I'll work on it once I correct my current pull.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 25, 2013

Member

@benjaminvialle @NelleV
What do you both think of doing something like this: danielstjules@9305613
The idea is that helper would handle array creation at the key for that flash type (:error, :notice, etc). This way devs don't have to remember where/when/if to create an array to hold the flash messages. They just call

flash_message :error, 'Text to display'

Then in the templates, you'd have...

    <% if flash[:success] -%>
        <p class="success"><%= flash[:success].join("<br />") %></p>
    <% end -%>
    <% if flash[:error] -%>
        <p class="error"><%= flash[:error].join("<br />") %></p>
    <% end -%>
    # etc...

That would then allow us to have multiple messages of each type.

The alternatives would be to :

B) Define each flash key (:error, :success, :notice, :warning) as an array at the beginning of a controller somehow, and that way you're always just pushing text on the array

C) Define each flash key as an array within each method that might use flash. I'd like to avoid this option, as if a dev forgets to define it as an array, the call to join from the erb will spit out a NoMethod Error. Or, to avoid this concern, we'd have to include a conditional statement in the template checking if flash[:type] is an array or a string, which isn't a very clean solution.

Member

danielstjules commented Jan 25, 2013

@benjaminvialle @NelleV
What do you both think of doing something like this: danielstjules@9305613
The idea is that helper would handle array creation at the key for that flash type (:error, :notice, etc). This way devs don't have to remember where/when/if to create an array to hold the flash messages. They just call

flash_message :error, 'Text to display'

Then in the templates, you'd have...

    <% if flash[:success] -%>
        <p class="success"><%= flash[:success].join("<br />") %></p>
    <% end -%>
    <% if flash[:error] -%>
        <p class="error"><%= flash[:error].join("<br />") %></p>
    <% end -%>
    # etc...

That would then allow us to have multiple messages of each type.

The alternatives would be to :

B) Define each flash key (:error, :success, :notice, :warning) as an array at the beginning of a controller somehow, and that way you're always just pushing text on the array

C) Define each flash key as an array within each method that might use flash. I'd like to avoid this option, as if a dev forgets to define it as an array, the call to join from the erb will spit out a NoMethod Error. Or, to avoid this concern, we'd have to include a conditional statement in the template checking if flash[:type] is an array or a string, which isn't a very clean solution.

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Jan 25, 2013

Member

Hi @danielstjules
I llike your idea. It looks very interresting to me. And your patch looks very clean.

What do you think @NelleV ?

Member

benjaminvialle commented Jan 25, 2013

Hi @danielstjules
I llike your idea. It looks very interresting to me. And your patch looks very clean.

What do you think @NelleV ?

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Feb 5, 2013

Member

An issue I hadn't considered is that we sometimes display a flash inline with form elements, and not simply at the top of the page.

Member

danielstjules commented Feb 5, 2013

An issue I hadn't considered is that we sometimes display a flash inline with form elements, and not simply at the top of the page.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Apr 7, 2013

Member

Forgot I never replied to this with my findings. Essentially, the idea I proposed before would only work if we differentiated between messages displayed in the headers, and left those displayed inline as being defined individually. It would clean things up a bit. I haven't been able to think of an alternative solution.

Member

danielstjules commented Apr 7, 2013

Forgot I never replied to this with my findings. Essentially, the idea I proposed before would only work if we differentiated between messages displayed in the headers, and left those displayed inline as being defined individually. It would clean things up a bit. I haven't been able to think of an alternative solution.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Aug 12, 2013

Member

@benjaminvialle @ghigt Do either of you have any suggestions for organizing this?

Member

danielstjules commented Aug 12, 2013

@benjaminvialle @ghigt Do either of you have any suggestions for organizing this?

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Aug 12, 2013

Member

Hi @danielstjules
I think your proposal is a very good idea. To my mind, you should add the definition of flash messages in application helper, as this helper is always available for controllers. Then, you won't have to define flash messages in every helper.

For forms flash messages, I don't know how to do that.

Could you create a PR with your modifications. I'll be happy to merge it!

Thank you

Member

benjaminvialle commented Aug 12, 2013

Hi @danielstjules
I think your proposal is a very good idea. To my mind, you should add the definition of flash messages in application helper, as this helper is always available for controllers. Then, you won't have to define flash messages in every helper.

For forms flash messages, I don't know how to do that.

Could you create a PR with your modifications. I'll be happy to merge it!

Thank you

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