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

Use application layout as default layout #1357

Merged
merged 3 commits into from
Apr 12, 2017
Merged

Use application layout as default layout #1357

merged 3 commits into from
Apr 12, 2017

Conversation

jessieay
Copy link
Contributor

WHY:

  • This is more standard in Rails
  • Before, we were using card everywhere, which also rendered
    application
  • This also slightly refactors how we deal with rendering flash
    messages. We should only be showing standard flashes. Others are used
    to pass around boolean values but do not need to be rendered.

**WHY**:
* This is more standard in Rails
* Before, we were using `card` everywhere, which also rendered
  `application`
* This also slightly refactors how we deal with rendering flash
  messages. We should only be showing standard flashes. Others are used
  to pass around boolean values but do not need to be rendered.
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, seems like a reasonable refactor

@@ -0,0 +1,4 @@
- if flash.any?
- flash.to_hash.slice("alert", "error", "notice", "success", "warning").each do |type, message|
Copy link
Contributor

Choose a reason for hiding this comment

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

should we put these keys in a constant somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONe at e28bf20

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 🎉 🎉

@jessieay jessieay merged commit e17bd0d into master Apr 12, 2017
@jessieay jessieay deleted the jy-card-layout branch April 12, 2017 17:45
@@ -1,6 +1,8 @@
class ApplicationController < ActionController::Base
include UserSessionContext

FLASH_KEYS = %w[alert error notice success warning].freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using the add_flash_types Rails convention instead? http://api.rubyonrails.org/classes/ActionController/Flash/ClassMethods.html

That way, we won't need to slice this array in shared/_flashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, works for me! create an issue for that?

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

Successfully merging this pull request may close these issues.

3 participants