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

Add page_description helper, refactor internals #47

Closed
wants to merge 1 commit into from

Conversation

croaky
Copy link
Contributor

@croaky croaky commented Mar 21, 2014

  • Use isolated testing style, not dummy app in spec suite.
  • Add page_description helper similar to page_title.
  • Refactor internals to have a single Flutie::ViewHelper.
  • Have page_title and page_description use i18n keys instead of
    content_for.
  • Document changes.

This is basically a complete rewrite and would introduce breaking changes for the page_title helper. I imagine we'd release a 3.0.0 version.

module Flutie
module ViewHelper
def body_class(options = {})
extra_body_classes_symbol = options[:extra_body_classes_symbol] || :extra_body_classes
Copy link

Choose a reason for hiding this comment

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

Did you mean options[:extra_body_classes_symbol] || options[:extra_body_classes]? If not, then fetch would be a good candidate here.

@gylaz
Copy link

gylaz commented Mar 22, 2014

This is great! 👍

@mcmire
Copy link

mcmire commented Mar 22, 2014

Looks good to me. I definitely like that page_title and page_description are stored in i18n.

* Use isolated testing style, not dummy app in spec suite.
* Add `page_description` helper similar to `page_title`.
* Refactor internals to have a single `Flutie::ViewHelper`.
* Have `page_title` and `page_description` use i18n keys instead of
  `content_for`.
* Document changes.
@croaky
Copy link
Contributor Author

croaky commented Mar 23, 2014

/cc @mjankowski


<title>Appname : page title</title>
By default, Flutie will create empty tags:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have to look at the HTML spec, but I believe that the title element cannot be empty.

I realize the intention of flutie is to have a reasonable default and assume that the user would override the default, but it seems weird to generate invalid html by default.

That was part of the motivation of the previous impl using the Rails app name (which, granted, is usually not a good page title, but was the least bad thing I could think of).

@croaky croaky closed this Feb 10, 2017
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