RFC: Admin configurable tabs

[kennyadsl]

Description

Following the discussion in #3233, I'm working on this spike to allow easy customization of admin tabs for stores and extensions, without using Deface.

This proposed structure is just a working attempt to programmatically control the tabs in the product page. If it works for everyone, we can extend it to all the other pages that use tabs.

Suppose you want to add a new tab for a product reviews section, that you implemented custom in your store.

You can create a class with your logic, similar to this one:

# lib/spree/backend/tabs/product/reviews.rb

require 'spree/backend/tabs/product/base'

module Spree
  module Backend
    module Tabs
      class Product
        class Reviews < Base
          def name
            'Reviews'
          end

          def presentation
            view_context.t('your_app.reviews')
          end

          def url
            view_context.spree.admin_product_reviews_url(product)
          end

          def visible?
            view_context.can?(:admin, Spree::Review)
          end
        end
      end
    end
  end
end

Next step is adding this class to the list of tabs to load:

# config/initializer/spree.rb

Spree::Backend::Config.configure do |config|
  config.product_tabs.items << 'Spree::Backend::Tabs::Product::Reviews'
end

🗣Any feedback in this early stage is highly appreciated.

TODO

• [ ] Wait for feedback
• [ ] Write tests
• [ ] Add remaining tabs
• [ ] Write documentation for other tabs or write a generic documentation for tabs
• [ ] Understand if we can deprecate the usage of data-hook="admin_product_tabs" via Deface
• [ ] Consider backporting to supported versions, so we can remove deface overrides in extensions for tabs

Checklist:

• [ ] I have followed Pull Request guidelines
• [ ] I have added a detailed description into each commit message
• [ ] I have updated Guides and README accordingly to this change (if needed)
• [ ] I have added tests to cover this change (if needed)

[kennyadsl]

@tvdeyen thanks, all your feedback makes sense. I'll try to rewrite this following your suggestion and other internal notes that I received.

[kennyadsl]

I've made a couple of other attempts:

v2

This follows the @tvdeyen suggestions of not using a view_context. I've created another object called TabsContext that stores all the information shared between tabs in the same tabs set. I think that the advantage of having a specific object to store context data is that we don't have to explicitly pass all data to all tabs every time.

I still think this has a big issue, the main object of the tabs context, in this case the Spree::Product instance, could be more than one object for other tabs sets, for example in the configuration pages that have tabs but are not related to .a specific object.

v3

This is similar to the approach used for MenuItems and it's probably the approach that I like the most at this point. We inject instances of Spree::Backend::Tabs into the configuration array and use them in the view. I think this work, and it's more flexible but I'm not sure about the extensive usage of instance_eval in the views.

Any thoughts?

Thought

[tvdeyen]

@kennyadsl I also prefer the v3. Much easier and instance_exec is a good trade-off here. I left some comments on the commit itself. Somehow GitHub is not displaying them here. Hope they reached you?

[waiting-for-dev]

Hey, @kennyadsl, it'd be nice to have this one. Pinging you in case you find the time to complete it 🙂

[chrean]

Closing because we are in the process of completely redoing the admin, starting from a UX design.