Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Commit

Permalink
Flashy messages don't disappear
Browse files Browse the repository at this point in the history 
Fixes issue #33
  • Loading branch information
@flavio
flavio committed May 7, 2015
1 parent de1a0eb commit 83d5268
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions app/assets/javascripts/application.js.coffee
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,10 @@
//= require turbolinks
//= require twitter/bootstrap
//= require_tree .

$(document).on "page:change", ->
$('#notice .close').on 'click', (event) =>
$('#notice').hide()
$('#alert .close').on 'click', (event) =>
$('#alert').hide()

4 comments on commit 83d5268

@mssola
Copy link
Collaborator

@mssola mssola commented on 83d5268 May 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding flashy messages, I figured that the problem was that the close button should have the #notice element as its direct parent. If this is the case, then bootstrap's alert will do the magic for us.

Now, since we are doing this manually, maybe it would be a good idea to remove some bootstrap-specific attributes for notifications. For example, we could remove data-dismiss="alert" from the close button if I'm not mistaken.

@flavio
Copy link
Member Author

@flavio flavio commented on 83d5268 May 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... What about fixing the code to get rid of the js code I wrote and let bootstrap do the dirty job? The less code we write, the less stuff we have to maintain :)

@mssola
Copy link
Collaborator

@mssola mssola commented on 83d5268 May 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is the way to go. But since I don't want to complicate the PR from your UI branch any longer, I think that it's better to create a new issue about this so we can fix it on another PR.

What do you think ?

@flavio
Copy link
Member Author

@flavio flavio commented on 83d5268 May 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll merge the PR. Could you please create a dedicated issue?

Please sign in to comment.