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

Template whitelist for page tabs, so tabs only appears on pages with certain templates #1753

Closed
wants to merge 3 commits into from

Conversation

jokklan
Copy link
Contributor

@jokklan jokklan commented Jun 13, 2012

@travisbot
Copy link

This pull request fails (merged 3a19cc4 into 3386fcd).

@jokklan
Copy link
Contributor Author

jokklan commented Jun 13, 2012

adding tests, if this is the approach you want to use.

@@ -17,6 +21,8 @@ def self.register(&block)

raise "A tab MUST have a name!: #{tab.inspect}" if tab.name.blank?
raise "A tab MUST have a partial!: #{tab.inspect}" if tab.partial.blank?
tab.templates = ['all'] if tab.templates.blank?
tab.templates = [tab.templates] unless tab.templates.is_a?(Array)
Copy link
Member

Choose a reason for hiding this comment

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

You can use this instead:

tab.templates = Array(tab.templates)

@parndt
Copy link
Member

parndt commented Jun 20, 2012

@ugisozols what do you think of this approach?

@ugisozols
Copy link
Member

Currently this won't work because tabs_for_template assumes @tabs is already set which it is not.

How we configure which tabs to use for page?

@@ -17,6 +21,8 @@ def self.register(&block)

raise "A tab MUST have a name!: #{tab.inspect}" if tab.name.blank?
raise "A tab MUST have a partial!: #{tab.inspect}" if tab.partial.blank?
tab.templates = ['all'] if tab.templates.blank?
Copy link
Member

Choose a reason for hiding this comment

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

I know it's nitpicky but I prefer this:

tab.templates = %w[all] if tab.templates.empty?

@jokklan
Copy link
Contributor Author

jokklan commented Jun 27, 2012

@tabs should be set. Extensions registrer tabs and add it to the tabs collection, stored in @tabs at initialization. Or have i missed something?
Another problem though is that templates is not set for pages (default is nil).

This is however just something i made quickly because, as i said, im not sure of the best approach. Template does however seems like an appropriate condition to determine the tabs to be used, or what do you think?

@travisbot
Copy link

This pull request fails (merged c99262d into 3386fcd).

@travisbot
Copy link

This pull request fails (merged a1ea456 into 3386fcd).

@jokklan
Copy link
Contributor Author

jokklan commented Jun 27, 2012

That's strange... It passes locally, for me.

@parndt
Copy link
Member

parndt commented Jul 6, 2012

if you look at the build you'll see only one build failed on jruby-18mode

@parndt
Copy link
Member

parndt commented Jul 6, 2012

Seems like a good strategy - can you add tests please?

@jokklan
Copy link
Contributor Author

jokklan commented Jul 10, 2012

Sure, but im currently on vacation. I will get to it as soon as i come home (about 2 weeks), if you can wait that long :)

@robyurkowski
Copy link
Contributor

Hey, @jokklan — any plan to revisit this?

@parndt
Copy link
Member

parndt commented Aug 23, 2012

I'd like to get this feature merged - anyone keen to tackle tests so that we don't break it in the future?

@ugisozols
Copy link
Member

Closing this in favour of #1943.

@ugisozols ugisozols closed this Sep 13, 2012
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.

5 participants