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

Show only alerts with supported types #526

Merged
merged 1 commit into from
Feb 12, 2013
Merged

Show only alerts with supported types #526

merged 1 commit into from
Feb 12, 2013

Conversation

rsamoilov
Copy link
Contributor

Flash messages are not something, that should always be displayed as an alert.

Flash can be used to store some service values or states and it's not expected behavior to show that values as alerts.
For example, if I want to render some form with default values from another action, I could use flash to store that values.

@toadkicker
Copy link
Contributor

I don't want to lock people into four types of statuses.

seyhunak added a commit that referenced this pull request Feb 12, 2013
Show only alerts with supported types
@seyhunak seyhunak merged commit 85f25fa into seyhunak:master Feb 12, 2013
@toadkicker
Copy link
Contributor

I don't like this commit because flash shouldn't be used for storing service values or states. Just because Twitter Bootstrap uses the standard logging level names to style the alert box doesn't mean we only check for those four statuses. It makes the gem less flexible and doesn't allow developers to use custom alert levels.

@rsamoilov
Copy link
Contributor Author

Why not? It's a common case to use flash for states.
I didn't dig a lot into sources, but it looks like devise uses flash in such way too - https://github.com/plataformatec/devise/blob/master/lib/devise/failure_app.rb#L80

If user needs a couple of warnings on page, with your implementation he could do it with something
like this: flash[:warning] = ['warning 1', 'warning 2']

@toadkicker
Copy link
Contributor

If I want flash[:whatever] to be styled like an error alert, this commit forces me to use flash[:error] to get it to display. The flash method accepts any symbol, and a library shouldn't stop a developer from doing that while still getting the UI to do what it wants. It's not about multiple messages, its about allowing the flash method to work as expected.

@rsamoilov
Copy link
Contributor Author

The flash method accepts any symbol, and a library shouldn't stop a developer from doing that

Exactly. And with previous implementation library stopped developer from using flash to store some service values, which are not desirable to be displayed.

This commit could be inverted - display all messages from flash except messages under some key, :service_values for example. But I personally don't see reason, which could force me to store error message in flash[:hello] or flash[:whatever]

This could be meaningful for displaying a couple of errors. So, I could write flash[:first_error], flash[:second_error] = 'error 1', 'error 2' and these errors would have not been displayed. But with your implementation I would write flash[:error] = ['error 1', 'error 2']. That's what I was talking about.

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.

None yet

4 participants