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

More h tags #4

Closed
wants to merge 2 commits into from
Closed

More h tags #4

wants to merge 2 commits into from

Conversation

pedz
Copy link

@pedz pedz commented Feb 2, 2013

h2 through h5 were not in COMPLETE

@rf-
Copy link
Owner

rf- commented Feb 3, 2013

Thanks for pointing out the inconsistency here. It's not necessary to list tags in COMPLETE that are already in BASIC, since the BASIC tags are already defined on every presenter. I removed the existing duplicates, which should hopefully make things less confusing: 0b014cf

The other commit doesn't quite do what you want. Since the self in the default parameter value is referring to the Keynote::Rumble module and not to the presenter class, it'll define the extra tag methods on every presenter, not just the one you call the method from. Instead, I added a use_html_5_tags method to presenter classes, so now you can just invoke that in the class body to get the extra tags: 577ed03

Thanks again for the code and feedback.

@rf- rf- closed this Feb 3, 2013
@pedz
Copy link
Author

pedz commented Feb 3, 2013

Yea. I see. In 0.2, I think I needed the h2 tags in both because of the way I hacked the code. I just used COMPLETE instead of BASiC at create time. Now in 0.3, the html 5 tags are added as a second set.

As far as the 2nd change (for self), I wasn't sure what would happen so I just tried it.

class ApplicationPresenter < Keynote::Presenter
Keynote::Rumble.use_html_5_tags

But now I see that it is probably just adding the tags to Rumble and Rumble is then included. So it will affect the entire application and not just the specific class. In my case, thats fine but others may be a bit surprised.

By the way, I know this wasn't your original intent but what I'm doing is this:

class MyController < ApplicationController
respond_to :html, :json

def show
...
respond_with(create_presenter(model))
end

Then I can put the to_json in the presenter rather than the model. I added a presenter helper so the views do not reference any instance variables directly. It makes the controllers and the view templates very trivial and easy to test.

I'm having great fun and Keynote is helping in that area.

On Feb 2, 2013, at 11:22 PM, Ryan Fitzgerald wrote:

Thanks for pointing out the inconsistency here. It's not necessary to list tags in COMPLETE that are already in BASIC, since the BASIC tags are already defined on every presenter. I removed the existing duplicates, which should hopefully make things less confusing: 0b014cf

The other commit doesn't quite do what you want. Since the self in the default parameter value is referring to the Keynote::Rumble module and not to the presenter class, it'll define the extra tag methods on every presenter, not just the one you call the method from. Instead, I added a use_html_5_tags method to presenter classes, so now you can just invoke that in the class body to get the extra tags: 577ed03

Thanks again for the code and feedback.


Reply to this email directly or view it on GitHub.

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.

2 participants