Use and extend view components in Solidus Admin #5099

waiting-for-dev · 2023-05-26T14:50:07Z

Summary

We're introducing ViewComponent for the new admin along with proposing a way to extend them from the host app.

Please, go through each commit description for a more complete walk-through.

As a recap, admin components can be replaced by the host app by creating a new component with a matching path within the app/components/{app_name}/solidus_admin/ directory. Example:

# app/components/sandbox/solidus_admin/main_nav_item_component.rb

module Sandbox
  module SolidusAdmin
    class MainNavItemComponent < ViewComponent::Base
      with_collection_parameter :item
  
      attr_reader :item
  
      def initialize(item:)
        @item = item
      end
  
      erb_template <<~ERB
        <li style="background-color: red">
          <a href="#">
            <%= item.title %>
          </a>
        </li
      ERB
    end
  end
end

In the case of desired fine-grained control, the created component can inherit the original one. For instance, here we're just changing the template while keeping all the rest of view logic in place (and the opposite could also be possible):

# app/components/sandbox/solidus_admin/main_nav_item_component.rb

module Sandbox
  module SolidusAdmin
    class MainNavItemComponent < ::SolidusAdmin::MainNavItemComponent
      erb_template <<~ERB
        <li style="background-color: red">
          <a href="#">
            <%= item.title %>
          </a>
        </li
      ERB
    end
  end
end

Unfortunately, view_component doesn't allow overriding templates from the host application by just having the same relative path. For now, we'll use the inline templates feature

Worth noting that we're injecting nested components as initialization dependencies for their parent components instead of going with slots. That's to avoid the problem with deeply nested slots and also to make customization easier by just overriding the initializer if needed:

# app/components/sandbox/solidus_admin/main_nav_component.rb

module Sandbox
  module SolidusAdmin
    class MainNavComponent < ::SolidusAdmin::MainNavComponent
      def initialize(main_nav_item_component: component("my_main_nav_item"), **kwargs)
        super
      end
    end
  end
end

``
The same registry that is used to keep track of the components is also used to contain data entities, like the one used to wrap the main navigation items. For instance, a new nav item could be registered like this:

require "solidus_admin/nav_item"
stats = SolidusAdmin::Stats.new(title: "Stats", position: 15)
SolidusAdmin::Container.register("main_nav.stats", stats)

Not showcased now, but we can also use the registry for the nav item itself, so users would have also full control over them.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed:

📖 I have updated the README to account for my changes.
📑 I have documented new code with YARD.
🛣️ I have opened a PR to update the guides.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.

tvdeyen

Yeah view_component 🎉

I really like that we are going this route 🥳

First of all I want to emphasize the great writing and git work on this PR. This is how PRs and commits should be written. It has been a pleasure to review and future versions of ourselves will thank you for this. 🎖️

I have some thoughts on naming.

tvdeyen · 2023-05-26T16:48:01Z

admin/lib/solidus_admin/container.rb

+  #
+  # We use this container to register all the components that can be
+  # overridden by the host application.
+  class Container < Dry::System::Container


Can we namespace this into Components, so that it is easy to understand what this container is used for?

SolidusAdmin::Components::Container

The issue is that, probably, we'll use the container for registering other kinds of dependencies. For instance, if, at some point, we introduce service objects only for the admin panel, they would also need to be registered here. In fact, we're already using it for an entity-like structure: the data wrapped within a SolidusAdmin::MainNavItem shouldn't be seen as something that only belongs to components.

tvdeyen · 2023-05-26T16:49:57Z

admin/lib/solidus_admin/main_nav_item.rb

+
+module SolidusAdmin
+  # Encapsulates the data for a main nav item.
+  class MainNavItem


Should we consider a SolidusAdmin::Components namespace for better code organisation?

As pointed in #5099 (comment), a MainNavItem is just something similar to a data object. We probably don't want to see it as something that can only belong to a view component. For instance, if we decide to persist them at some point, they could also be stored as database records.

tvdeyen · 2023-05-26T16:54:37Z

admin/app/components/solidus_admin/main_nav_component/main_nav_component.html.erb

@@ -1,6 +1,6 @@
 <nav>
  <%=
-    render SolidusAdmin::MainNavItemComponent.with_collection(
+    render container["main_nav_item_component"].with_collection(


Maybe use a more descriptive name?

components_container[...]

or even better, remove the implementation detail (that this is an Dry::Container instance) and rename it into something that represents its usecase instead?

admin_components[...]

or short?

components[...]

Fixed in subsequent commits 🙂

tvdeyen · 2023-05-26T16:55:31Z

admin/app/helpers/solidus_admin/container_helper.rb

+module SolidusAdmin
+  module ContainerHelper
+    def container
+      SolidusAdmin::Container


Container is very technical. Can we find a name that represents its purpose?

Also, I think we can make container private and could add a components() method like we did with component()?

I also have some doubts about the name, what do you think about SolidusAdmin::Registry?

tvdeyen · 2023-05-26T16:57:24Z

admin/app/components/solidus_admin/base_component.rb

@@ -6,5 +6,9 @@ class BaseComponent < ViewComponent::Base
    def container
      SolidusAdmin::Container
    end
+
+    def component(name)


Nice. Never mind my previous comments :)

Can we make container private then?

See #5099 (comment)

tvdeyen · 2023-05-26T17:03:59Z

admin/app/components/solidus_admin/main_nav_component.rb

+      <nav>
+        <%=
+          render component("main_nav_item").with_collection(
+            container.all("main_nav").sort_by(&:position)


What do you think about?

components.all("main_nav")...

as helper for the container implementation detail?

See #5099 (comment)

tvdeyen · 2023-05-26T17:06:24Z

admin/app/components/solidus_admin/base_component.rb

-    def component(name)
-      container.resolve("#{name}_component")
-    end
+    include SolidusAdmin::ContainerHelper


What about SolidusAdmin::ComponentsHelper? I would like to avoid adding a technical implementation detail to our public API. I prefer naming that explains its usecase rather than its implementation.

See #5099 (comment)

tvdeyen · 2023-05-26T17:11:21Z

admin/app/helpers/solidus_admin/container_helper.rb

+      SolidusAdmin::Container
+    end
+
+    def component(name)


👏🏻 I like this as this hides the implementation.

tvdeyen · 2023-05-26T17:13:52Z

admin/lib/solidus_admin/system/providers/main_nav.rb

+require "solidus_admin/main_nav_item"
+
+module SolidusAdmin
+  module Providers


Maybe a Components namespace would help here as well to understand the usecase of this "Provider" a bit better?

See #5099 (comment)

tvdeyen · 2023-05-26T17:21:12Z

admin/app/components/solidus_admin/main_nav_component.rb

+module SolidusAdmin
+  # Renders the main navigation of Solidus Admin.
+  class MainNavComponent < BaseComponent
+    def initialize(main_nav_item_component: component("main_nav_item"), items: container.within_namespace("main_nav"))


If we would add a components helper that uses container.within_namespace internally, we could make this much more userfriendly and would hide the technical detail of a Dry::Container from the user.

Suggested change

def initialize(main_nav_item_component: component("main_nav_item"), items: container.within_namespace("main_nav"))

def initialize(main_nav_item_component: component("main_nav_item"), items: components("main_nav"))

WDYT?

Please, notice that container.within_namespace("main_nav") is not resolving ViewContainer classes nor instances, but instances of SolidusAdmin::MainNavItem entities. I don't think it's a big problem exposing the container (happy to rename it to registry) to the components, as it's precisely intended to resolve dependencies. However, I agree that there's a problem having it also available in the templates, but I can't see a way to avoid that, as it's more of a view_component limitation. What do you think?

On top of that, I'd love to use a more declarative specification of dependencies via dry-system's auto-injection feature. However, that would require having to call super everytime we define the initialize method for a component. I didn't go that way because I think it's good to stick as much as we can to bare view_components so that ours users don't have a hard time extending our components.

module SolidusAdmin class MainNavComponent < BaseComponent include Import["main_nav_item_component"] def initialize(items: container.within_namespace("main_nav")) super @items = items end erb_template <<~ERB <nav> <%= render main_nav_item_component.with_collection( sorted_items ) %> </nav> ERB private def sorted_items @items.sort_by(&:position) end end end

Finally, I also added auto-injection to this spike. In the end, super needs to be called whenever you override an initializer, and rubocop will complain if you don't.

waiting-for-dev · 2023-05-29T11:40:56Z

I really like that we are going this route partying_face

Happy to hear that! I hope our broader community will feel the same 🙂

First of all I want to emphasize the great writing and git work on this PR. This is how PRs and commits should be written. It has been a pleasure to review and future versions of ourselves will thank you for this. medal_military

Thanks for your nice words 🙇 🙂

kennyadsl

Thanks @waiting-for-dev, I left some questions before approving, but I'm overall excited to merge this!

admin/app/components/solidus_admin/main_nav_component.rb

admin/lib/solidus_admin/container.rb

admin/lib/solidus_admin/system/providers/main_nav.rb

kennyadsl

Approving, but there's an issue with the installer related to the version of the solidus_admin. Do we already have a fix for that in mind?

waiting-for-dev · 2023-05-30T14:45:37Z

Approving, but there's an issue with the installer related to the version of the solidus_admin. Do we already have a fix for that in mind?

Not sure what's going on. I tried to reproduce locally but it worked. I'll look into it.

codecov · 2023-05-31T03:47:00Z

Codecov Report

Merging #5099 (bedf820) into nebulab/admin (9ffffae) will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           nebulab/admin    #5099   +/-   ##
==============================================
  Coverage          88.62%   88.62%           
==============================================
  Files                563      563           
  Lines              13882    13882           
==============================================
  Hits               12303    12303           
  Misses              1579     1579

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

waiting-for-dev · 2023-05-31T03:48:47Z

Approving, but there's an issue with the installer related to the version of the solidus_admin. Do we already have a fix for that in mind?

@kennyadsl, bumping the cache version on the CI to fetch the latest changes on SSF to allow view_components ~> 3.0 fixed the issue (ref: solidusio/solidus_starter_frontend#343)

elia

It's great to see this coming along!

Left a couple of comments, mainly food for thought, happy to discuss it with a shared screen!

elia · 2023-05-31T09:53:52Z

admin/app/views/layouts/solidus_admin/application.html.erb

@@ -5,6 +5,7 @@
  </head>
  <body>
    <h1 class="text-4xl">Layout is rendered</h1>
+    <%= render component("main_nav").new %>


Maybe I missing some benefits/features provided by DRY, but I'm left wondering if we can go a little bit more vanilla.

Which would be the pros/cons of going with something dumber like the following?

(this is meant in terms of approach, to be considered just pseudo code)

<%= render component("main_nav").new(items: SolidusAdmin::NavItem.all) %>

class SolidusAdmin::NavItem < ActiveModel::Model def self.all @all ||= SolidusAdmin::Config.nav_items.map { new _1 } end end

# initializers/solidus_admin.rb SolidusAdmin::Config.nav_items += [ { name: "Extra item", icon: :gift, … } ]

Hey @elia, I pointed out some of the befits and features that dry-system provides in another answer.

<%= render component("main_nav").new(items: SolidusAdmin::NavItem.all) %>

I think it's good having the default value for items already defined on the initializer of MainNavComponent. In this case, it doesn't make such a difference, but maybe other components will be reused, which would avoid boilerplate code. In that situation, dry-system's auto_inject would also help.

class SolidusAdmin::NavItem < ActiveModel::Model
def self.all
@ALL ||= SolidusAdmin::Config.nav_items.map { new _1 }
end
end

I know that's not a formal proposal, but regardless of using dry-system or not, I'd go with a PORO for nav item entities. In the end, we don't need to deal with validations, which would be a case for ActiveModel::Model.

SolidusAdmin::Config.nav_items += [
{ name: "Extra item", icon: :gift, … }
]

I'd also like to avoid having SolidusAdmin::Config as a hodgepodge. For anything else that configuring simple primitives, I think it'd be good having a richer interface, whether that one is provided by dry-system or something we own.

@elia, please, take a look at the last commit I pushed (Auto-inject dependencies into components). I also included dry-sytem's auto_inject within this spike, to better showcase both how having defaults defined in the #initialize method, and having nav items registered in the container help with avoiding boilerplate. Also pinging @kennyadsl.

elia · 2023-06-01T08:00:00Z

admin/lib/solidus_admin/container.rb

+    configure do |config|
+      config.root = Pathname(__FILE__).dirname.join("../..").realpath
+      config.component_dirs.add("app/components") do |dir|
+        dir.loader = System::Loaders::HostOverridableConstant.method(:call).curry["components"]
+        dir.namespaces.add "solidus_admin", key: nil
+      end
+    end


Thoughts on using a simple hash here, that would require an explicit configuration like the ones we have for replacing classes in Core?

E.g.

SolidusAdmin::Config.components_registry['main_nav'] = 'MyStore::SuperCoolNavComponent'

def components_registry @components_registry ||= Hash.new do |_hash, key| "solidus_admin/components/#{key}_component".classify.constantize end end

Hey @elia, as pointed out in a previous comment, there's definitely nothing we couldn't build on our own. However, for the sake of this spike, I wanted to focus on the architecture and the interface to interact with it instead of getting lost in implementation details. dry-system allowed me to do that, but I have no problems revisiting it and evaluating if it's enough to build something like a Hash-based registry.

I'm not sure which direction we're going to take on other topics, like service objects on the new admin, but having dry-system in place can help us with moving and changing things faster, as its declarative configuration is very flexible. Then, it can also give us other valuable features like logging, monitoring, or testing support (stubbing a component while testing). We can assess how much of it we end up using, and then decide whether to go with or without it. What do you think?

Maybe it's unfamiliarity with DRY, but looks like for our case it's an additional layer of injections on top of the already sufficient initializer params, which I also find more explicit.

I think having a more complete context might help in appreciating pros and cons of each solution, thoughts on holding a little bit on merging and keeping this PR as a reference maybe coming back to it once we start implementing?

I think we can merge right away the first commits setting up the Base component class

After talking offline, we decided to merge everything now and re-evaluate along the way 👍

We need to bump CircleCI cache to fetch latest changes on SSF allowing view_components v3 (see solidusio/solidus_starter_frontend#343).

This is only to showcase the structure of a Solidus Admin component, where all components should inherit from the BaseComponent class.

…nent We're still on a showcase phase, showing now how components can be nested within other components.

We're transforming the main nav item component into a collection component to allow reusing it from within the main nav component. We're separating the data for a nav item from its view representation. A `SolidusAdmin::MainNavItem` represents a nav item, with its title and position. We're using `dry-system` to register the available nav items into the `SolidusAdmin::Container` registry. On top of that, host applications can register their own ones. The `main_nav` key is used to register items in the collection. All of them can be accessed through `SolidusAdmin::Container.within_namespace('main_nav')`. Example of how to register a nav item: ```ruby require "solidus_admin/main_nav_item" stats = SolidusAdmin::MainNavItem.new( title: "Stats", position: "15" ) SolidusAdmin::Container.register("main_nav.stats", stats) ``` Once we're out of the showcase phase, we can pass the URL as a lambda taking the request context.

@item

By always referencing them through the container, we're allowing host applications to inject their ones. For example: ```ruby require "solidus_admin/base_component" class MyNavItemComponent < SolidusAdmin::BaseComponent with_collection_parameter :item attr_reader :item def initialize(item:) @item = item.do_something end end Rails.application.config.to_prepare do SolidusAdmin::Container.register("nav_item_component", MyNavItemComponent) end ``` We're creating a custom dry-system loader so that we can load the component class instead of trying to initialize an instance.

As it's going to be something that we'll use a lot, it's better to have a helper to render components resolved from the container.

Unfortunately, view_component doesn't allow overriding templates from the host application by just having the same relative path [1]. For now, we'll use the inline templates feature from v3 [2] so that host applications can override only the template while still inheriting the rest of view logic. [1] - ViewComponent/view_component#411 [2] - https://viewcomponent.org/guide/templates.html#inline

This allows host apps to override the default admin components by defining a component with a matching path. For example, considering our component defined at `app/components/solidus_admin/main_nav_component.rb`, and given and application called `MyApp`, we can override the default component by defining a component at `app/components/my_app/solidus_admin/main_nav_component.rb`. This is done by adding a new loader to the system, which will look for components in the host app, and will inject them into the system. Host applications can still manually inject components by using the `SolidusAdmin::Container.register` method as before. This commit also renames the `providers` directory to `system/providers` to better organize the code.

By following this pattern, overriding the parent component becomes easier, as only the initializer is needed. That also allows to avoid slots and its problem with deeply nested structures [1]. [1] - ViewComponent/view_component#867

This is to avoid duplicated methods.

By using dry-system's auto_inject feature, we can get rid of the boilerplate code that injects dependencies into components. We create a `SolidusAdmin::Import` constant that can be used to dynamically create readers for each declared dependency. We also register a top level `main_nav_items` dependency to avoid calling a private in nature `.within_namespace` method. [1] - https://dry-rb.org/gems/dry-system/1.0/dependency-auto-injection/

waiting-for-dev requested a review from a team as a code owner May 26, 2023 14:50

github-actions bot added the changelog:repository Changes to the repository not within any gem label May 26, 2023

waiting-for-dev self-assigned this May 26, 2023

waiting-for-dev requested review from elia and rainerdema May 26, 2023 14:50

tvdeyen reviewed May 26, 2023

View reviewed changes

waiting-for-dev force-pushed the waiting-for-dev/extending_components branch from 0d09441 to b790e3a Compare May 29, 2023 11:22

kennyadsl reviewed May 30, 2023

View reviewed changes

admin/app/components/solidus_admin/main_nav_component.rb Show resolved Hide resolved

admin/lib/solidus_admin/container.rb Show resolved Hide resolved

admin/lib/solidus_admin/system/providers/main_nav.rb Show resolved Hide resolved

kennyadsl approved these changes May 30, 2023

View reviewed changes

waiting-for-dev force-pushed the waiting-for-dev/extending_components branch from b790e3a to 13a48fe Compare May 31, 2023 03:34

waiting-for-dev force-pushed the waiting-for-dev/extending_components branch from 13a48fe to bd164f3 Compare June 1, 2023 03:28

github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels Jun 1, 2023

elia reviewed Jun 1, 2023

View reviewed changes

waiting-for-dev force-pushed the waiting-for-dev/extending_components branch from bd164f3 to c8570df Compare June 1, 2023 14:49

github-actions bot added the changelog:solidus Changes to the solidus meta-gem label Jun 1, 2023

waiting-for-dev added 9 commits June 5, 2023 05:07

Install view_component and make it available to SolidusAdmin

99fd36c

We need to bump CircleCI cache to fetch latest changes on SSF allowing view_components v3 (see solidusio/solidus_starter_frontend#343).

Dummy main navigation component for Solidus Admin

ddc596e

This is only to showcase the structure of a Solidus Admin component, where all components should inherit from the BaseComponent class.

Nest a component for menu items within Solidus Admin's main nav compo…

cf49184

…nent We're still on a showcase phase, showing now how components can be nested within other components.

Helper to render admin components resolved from the container

e502c75

As it's going to be something that we'll use a lot, it's better to have a helper to render components resolved from the container.

waiting-for-dev added 3 commits June 5, 2023 05:07

Include admin container helper from components

e838a8b

This is to avoid duplicated methods.

Add component paths to the admin's tailwind config

aa17044

Fix rubocop offenses

8150c02

waiting-for-dev force-pushed the waiting-for-dev/extending_components branch from c8570df to 8150c02 Compare June 5, 2023 03:07

github-actions bot removed changelog:solidus_core Changes to the solidus_core gem changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem labels Jun 5, 2023

waiting-for-dev requested a review from elia June 5, 2023 04:24

waiting-for-dev force-pushed the waiting-for-dev/extending_components branch from 5dbf45b to 57c75de Compare June 5, 2023 04:39

waiting-for-dev force-pushed the waiting-for-dev/extending_components branch from 57c75de to bedf820 Compare June 5, 2023 04:45

waiting-for-dev merged commit 80f2434 into nebulab/admin Jun 6, 2023

waiting-for-dev deleted the waiting-for-dev/extending_components branch June 6, 2023 14:43

debreczeni mentioned this pull request Dec 19, 2024

Customizing Backend section out of date solidusio/edgeguides#140

Open

	def initialize(main_nav_item_component: component("main_nav_item"), items: container.within_namespace("main_nav"))
	def initialize(main_nav_item_component: component("main_nav_item"), items: components("main_nav"))

Use and extend view components in Solidus Admin #5099

Use and extend view components in Solidus Admin #5099

Conversation

waiting-for-dev commented May 26, 2023 • edited Loading

Summary

Checklist

tvdeyen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev commented May 29, 2023

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

waiting-for-dev commented May 30, 2023

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

waiting-for-dev commented May 31, 2023

elia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev commented May 26, 2023 •

edited

Loading

tvdeyen left a comment •

edited

Loading

waiting-for-dev May 29, 2023 •

edited

Loading

codecov bot commented May 31, 2023 •

edited

Loading

waiting-for-dev Jun 5, 2023 •

edited

Loading