techiferous / tabulous

Easy tabbed navigation for Rails
Other
322 stars 35 forks source link

trouble generating active subtab for "show" action #44

Open ljulien opened 10 years ago

ljulien commented 10 years ago

I'm trying to generate subtabs dynamically. In this case, I have a limited number of Floors, and I would like to create a subtab pointing to the "show" action of the "maps" controller for each one of them.

I can successfully generate the subtabs like this:

    Floor.all.each do |floor|
      send("floor_map_#{floor.id}_subtab") do
        text          { "Floor #{floor.id}" }
        link_path     { map_path(floor) }
        visible_when  { true }
        enabled_when  { true }
        active_when   { (floor.id == @floor) and in_action('show').of_controller('maps') }
      end
    end 

but I can't successfully make the proper one active. I have set the @floor variable in the controller, but it doesn't seem to be set (or it is otherwise not available) when active_when is called.

My ( floor.id == @floor ) test works correctly in the enabled_when and visible_when blocks, but not in active_when. Since the parent tab is set to "active_when { a_subtab_is_active }" I really do need to make the proper tab active.

Any ideas?

Thanks!

techiferous commented 10 years ago

Hi @ljulien,

I'm sorry to say that tabulous was not designed with this use case in mind and so I think you may not be able to get this to work.

First of all, the ability to generate subtabs dynamically is not supported by tabulous, but I must say I am quite impressed with your approach to get it to work. Everything about your solution should work just fine except for the active_when declaration. The block passed to active_when is not called in the context of a view (like the other blocks). It is also not a boolean expression (like the other blocks) but instead it's a DSL for listing the actions that trigger the "active" state for this tab.

And this is the point where you are running up against a design decision of tabulous: active tabs are linked to the controller action. If you are wanting to have the tab's active state linked to something other than the controller action, then sadly, tabulous is not the tool for the job.

ljulien commented 10 years ago

Thanks, @techiferous.

I'd dug through the source before I wrote, so I feared it was exactly as you say, but I'd hoped I was missing something. I'm pretty committed to Tabulous (I've got it in heavy use across multiple applications in or organization), so I'll come up with something that works for this particular set of features.

My current solution is to set the parent tab to:

active_when   { in_action('any').of_controller('maps') }

and include only the dynamically-generated subtabs (and no other controller actions). Then I just don't display the "active" one, since visible_when works as I'd hoped. Not perfect, but definitely workable for now.

If I find some clever way to do exactly what I want, I'll let you know.

techiferous commented 10 years ago

Oh wow, being so committed to tabulous brings a tear to my eye. :smiley:

It might not be that hard to fork tabulous and extend the DSL yourself, assuming the metaprogramming isn't too unwelcoming. It could be something like this:

active_when do
    in_action('show').of_controller('maps')
    and_when { floor.id == @floor }
end

Then, in https://github.com/techiferous/tabulous/blob/master/lib/tabulous/dsl/tab.rb, you could do something like this:

    def and_when(&block)
      @tab.extra_active_conditions = block
    end

Then you could evaluate that block here: https://github.com/techiferous/tabulous/blob/master/lib/tabulous/tab.rb

Thanks for your enthusiasm for tabulous.

ljulien commented 10 years ago

Hey, Tabulous does the job, better than some, and more easily than many. I love the improvements to the configuration for 2.x (I wasn't a huge fan of the 1.x configuration format, but it worked).

Thanks for pointing me in the right direction! I've applied what you said far enough to show a functional proof-of-concept, but I won't have the time to make it clean/right/robust until next week at least. If/when I have the time to do that, I will send along a pull request.

Speaking of pull requests, I had monkey-patched Tabulous 1.x to take html options for links, since my users were clamoring for "Are you sure?" confirmations on some of the subtab actions...and once you allow for one html option, you might as well allow for arbitrary ones. When I got my release done and was ready to put together a pull request for you, I discovered that Tabulous 2.x had been released, and I dropped the ball again. If arbitrary link html options interests you, I'll try to put that together as well.

But anyway, yes, you've given me exactly what I needed. I'm sure I will be able to add conditions to active_when. Thanks!

techiferous commented 10 years ago

Thanks! By the way, feel free to submit experimental / half-implemented pull requests (as long as you're okay with them not being accepted for several weeks) because getting the tests fully set up for tabulous development can be a pain. I can add the final polish and tests when I have time.

As for the arbitrary link html options, unless I'm misunderstanding the need, I think writing a custom renderer should work for this.

ljulien commented 10 years ago

On 3/8/14 8:00 AM, Wyatt Greene wrote:

As for the arbitrary link html options, unless I'm misunderstanding the need, I think writing a custom renderer should work for this.

I am finally picking this up again, many months later (when I have chosen to update one of my apps from Tabulous 1.x to 2.x).

Unless I'm misunderstanding something, I don't see how to pass arguments to the renderer on a per-tab basis.

I don't want a custom renderer that asks for confirmation upon clicking any tab, I only want to ask for confirmation for very specific ones (like the "Delete this item right now" tab).

In my old Tabulous 1.x app, I had written a monkey patch that would let me specify that I wanted a particular tab to ask for confirmation, by setting "data-confirm" on the link to the specified text.

It appears that I still need to change the Tabulous:Tab Model and the Tabulous:Dsl:Tab to do this -- I need to add a new declaration for "confirm_text" or something like that, so that the renderer can know what to do.

I've got a new, 2.x-compatible monkey patch that does this for me, and I would be happy to convert it to a pull request and submit it to you. If I'm still going about this all wrong, please do point me in the right direction. Otherwise I'll get this thing cleaned up and submitted.

Thanks! Linda

techiferous commented 10 years ago

Another solution would be to attach JavaScript to that tab link that adds the confirmation behavior. That keeps you from having to monkeypatch tabulous.

You're welcome to fork tabulous but I'm shy about extending the DSL so I'm unlikely to merge a pull request until I have a sense that other people are wanting the behavior, too. I'm trying to keep the behavior of tabulous to the sweet spot of a simple core without being too limiting.

ljulien commented 10 years ago

On 11/5/14 8:39 PM, Wyatt Greene wrote:

Another solution would be to attach JavaScript to that tab link that adds the confirmation behavior. That keeps you from having to monkeypatch tabulous.

Well, yes, I thought about that...but it means I need to write javascript! ;-)

Though actually...I still need to mark certain specific tabs as special (i.e. "needs confirmation"), and again, unless I'm missing something, I don't see how to pass in a class or any other identifying thing about the tab that would mark it as different than the others. If I have to tell javascript to change it based on the URL or something like that, I really prefer the monkey patch.

You're welcome to fork tabulous but I'm shy about extending the DSL so I'm unlikely to merge a pull request until I have a sense that other people are wanting the behavior, too. I'm trying to keep the behavior of tabulous to the sweet spot of a simple core without being too limiting.

I hear what you're saying!

I think that if I were going to try to convince you to accept a pull request, I wouldn't just implement confirmation (which is the specific thing my users are clamoring for in this particular instance, and what my monkey patch does at the moment), but rather to extend it to support arbitrary link options, or maybe arbitrary extra options for the tab, so that the renderer can do as it will on the other side. That would be broadly useful, and I hope a much easier sell.

My challenge here is that I can't pass any information from the configuration through to the other side, which limits my ability to treat some tabs differently from others.

That being said, as always, I love Tabulous, and I've standardized on it for navigation for all of my apps. I sorta hated the format of the config file for 1.x, but you fixed it beautifully for 2.x, which is why I'm converting my oldest old-format app over. Tabulous is a pleasure to use, and it does almost everything I need. :-)

Thanks, Linda

techiferous commented 10 years ago

unless I'm missing something, I don't see how to pass in a class or any other identifying thing about the tab that would mark it as different than the others.

Ah, you're right! Out of the box, tabulous doesn't make it easy to apply CSS or JavaScript to a particular tab. Perhaps a good way to extend tabulous is to have it add a class name to each tab's markup based on the tab name. That way you could easily hook up JavaScript.

but rather to extend it to support arbitrary link options

This is an interesting idea. Not sure how I feel about it. The purist in me wants to treat tabs as strictly navigation, meaning all links should be GET requests. That probably eliminates a lot of the need for extra options.

or maybe arbitrary extra options for the tab, so that the renderer can do as it will on the other side.

Tabulous already allows you to write your own custom renderer which gives you complete control over the generated HTML. It's just not as easy as using the DSL.

I love Tabulous, and I've standardized on it for navigation for all of my apps.

Thanks! I work on tabulous in part as a gift to the world, so I'm really glad you are finding it useful!

ljulien commented 10 years ago

On 11/5/14 9:12 PM, Wyatt Greene wrote:

unless I'm missing something, I don't see how to pass in a class or
any other identifying thing about the tab that would mark it as
different than the others.

Ah, you're right! Out of the box, tabulous doesn't make it easy to apply CSS or JavaScript to a particular tab. Perhaps a good way to extend tabulous is to have it add a class name to each tab's markup based on the tab name. That way you could easily hook up JavaScript.

Well, yes, but that still doesn't let me apply a class to a, well, class of tabs...you know? For instance, some of my options, though peppered around my tab structure, are reserved for people with admin/root privileges, and maybe it might be nice to make them a different color or something.

I could certainly manage what I need if you added a name-based class (though maybe an id would be better than a class, since tab names are unique?), but I would feel like I was breaking an abstraction barrier to put such specific information about a single tab into my JS files.

but rather to extend it to support arbitrary link options

This is an interesting idea. Not sure how I feel about it. The purist in me wants to treat tabs as strictly navigation, meaning all links should be GET requests. That probably eliminates a lot of the need for extra options.

I didn't mean that, exactly.

For my own purposes, today I want to set "data-confirm" and I can definitely imagine wanting to apply a class of my choosing to specific tabs of my choosing. Maybe someone would like to set "target"?

For instance, I use tab navigation for a lot of "list all Foos, make a new Foo, edit Foo, destroy Foo." I pretty much always want to throw an "Are you sure?" in for my Destroy links, so being able to apply stuff to them as a group would help me a lot. I would prefer to keep that stuff in a single config file rather than having part of it hidden in a javascript file somewhere.

My general approach is to let the user pass through whatever arguments they want to the underlying stuff, and let them shoot themselves in the foot if they want to.

or maybe arbitrary extra options for the tab, so that the renderer
can do as it will on the other side.

Tabulous already allows you to write your own custom renderer which gives you complete control over the generated HTML. It's just not as easy as using the DSL.

Yes, but (and please tell me if I'm missing something), while I can write a custom renderer that gives me complete control over all HTML, for all tabs...but I can't tell those tabs apart, except by URL or controller actions. I don't have a way for the tell the renderer, from the config file, that I want it to render one tab differently than another.

I want the config file to be able to set something that the renderer can see and act upon.

Actually, can I ask you why you're using directly in your default renderer rather than link_to?

I love Tabulous, and I've standardized on it for navigation for all
of my apps.

Thanks! I work on tabulous in part as a gift to the world, so I'm really glad you are finding it useful!

...and a fine gift it is!

If I can find the copious spare time to fork Tabulous and do the more generic solution, you can take a look at it and see what you think. As it is, I don't always have the luxury of doing things as properly as I would like.

Again, thanks!

Linda

techiferous commented 10 years ago

My general approach is to let the user pass through whatever arguments they want to the underlying stuff, and let them shoot themselves in the foot if they want to.

Tabulous's approach is to be declarative and not expose anything. It tries to allow you to simply "state what you want" in the language of the problem domain, not the language of the solution domain. The nice thing about this approach is it's very economical: in a few lines of code you can get a lot of behavior that "just works". The downside, as you are finding out, is that if a behavior is not supported, you end up having to jump into the deep end to get something to work.

Yes, but (and please tell me if I'm missing something), while I can write a custom renderer that gives me complete control over all HTML, for all tabs...but I can't tell those tabs apart, except by URL or controller actions.

Actually, you can access the tab's name like this:

def tab_html(tab) 
  return super unless tab.name == 'pines'
  html = ''
  klass = 'this-is-a-pine-tab'
  klass << ' active' if tab.active?(@view)
  klass << ' disabled' unless tab.enabled?(@view)
  klass << ' dropdown' 
  klass.strip!
  if klass.empty?
    html << '<li>'
  else
    html << %Q{<li class="#{klass}">}
  end
  if tab.clickable?(@view)
    html << %Q{<a href="#{tab_url(tab)}" class="tab" data-confirm="Is it opposite day?" #{tab_http_verb_attributes(tab)}>#{tab_text(tab)}</a>}
  else
    html << %Q{<span class="tab">#{tab_text(tab)}</span>}
  end
  html << "</li>"
  html
end

So I think tabulous can actually support what you want to do by using a custom renderer, although it's a bit klunky.

Actually, can I ask you why you're using directly in your default renderer rather than link_to?

More control; closer to the metal. It's more apparent when you look at the bootstrap renderer.

I don't want to extend the DSL for this case; I want to keep it simple and tight. I see a need to make working with custom renderers more straightforward, though.

techiferous commented 10 years ago

That said--and now I'm going to contradict myself :wink:--I could see a way to extend the DSL in a way that keeps with the spirit of tabulous:

  unicorns_tab do
    text             'Unicorns'
    link_path        '/unicorns'
    link_verb        :post
    link_class       'unicorn-tab'
    link_attributes  'data-confirm="Are you a unicorn?" foo="bar"'
    visible_when     { current_user.loves_unicorns? }
    enabled_when     true
    active_when      { in_action('any').of_controller('unicorns') }
  end
ljulien commented 10 years ago

That was sort of what I was saying. ;-)

Thanks! Linda

On 11/6/14 7:11 PM, Wyatt Greene wrote:

That said--and now I'm going to contradict myself :wink:--I could see a way to extend the DSL in a way that keeps with the spirit of tabulous:

unicorns_tab do text 'Unicorns' link_path '/unicorns' link_verb :post link_class 'unicorn-tab' link_attributes 'data-confirm="Are you a unicorn?" foo="bar"' visible_when { current_user.loves_unicorns? } enabled_when true active_when { in_action('any').of_controller('unicorns') } end

— Reply to this email directly or view it on GitHub https://github.com/techiferous/tabulous/issues/44#issuecomment-62075283.

techiferous commented 10 years ago

Hopefully the custom renderer approach gets you unblocked from your current problem. Making custom renderers easier to work with and possibly extending the DSL are good ideas that I'll keep in mind.

ljulien commented 10 years ago

On 11/6/14 7:16 PM, Wyatt Greene wrote:

Hopefully the custom renderer approach gets you unblocked from your current problem. Making custom renderers easier to work with and possibly extending the DSL are good ideas that I'll keep in mind.

Well, not really. I don't want my custom renderer to know the names of my tabs, because that breaks an abstraction barrier that I'm not comfortable. I'm more comfortable doing the extensions I want in a monkey-patch.

I've enclosed my hastily-thrown-together monkey patch for your perusal, so you can see what I'm talking about. This one just adds confirm_text and not arbitrary link options, but you get the idea.

Then my custom renderer only differs from the default one in that it add "data-confirm="#{tab.confirm_text}" to the link.

It works, and doesn't break abstraction barriers. Like I said, I would clean it up and make it more generically useful before submitting a pull request.

At least it got my release out today, which has quite a bit of value, right? :-)

Linda

techiferous commented 10 years ago

At least it got my release out today, which has quite a bit of value, right?

Yes! Glad you were able to get working software out of this. :smile: