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

Make application routes usable on example views #141

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nicolas-brousse
Copy link
Member

@nicolas-brousse nicolas-brousse commented Apr 3, 2019

If we want to have a component that works directly with a model object, Rails application returns an error.

With the following example.

# frontent/components/post_card/_examples.html.erb
<%= cdoc "post_card", post: Post.first %>
# frontent/components/post_card/_post_card.html.erb
<div class="post_card">
<%= link_to Post.name, post_path(post) %>
</div>

We have the following error:

undefined method `post_path' for #<#<Class:0x00007f579c3f6c90>:0x00007f579c36b668>

Explanation

The komponent gem is isolated so application routes are not available from Komponent::StyleguideController.

Proposal 1

The changes I did expose Rails application routes in Komponent::StyleguideController. And I named the engine to be able to access komponent application routes.

Proposal 2 (current code state of this PR)

Stop to isolate the engine. So routes are loaded in application. But helper will be loaded too.

@nicolas-brousse
Copy link
Member Author

I'm currently reading an article and some documentation to be sure of my proposal.

@coveralls
Copy link

coveralls commented Apr 3, 2019

Pull Request Test Coverage Report for Build 815

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 99.451%

Totals Coverage Status
Change from base Build 814: 0.003%
Covered Lines: 181
Relevant Lines: 182

💛 - Coveralls

@nicolas-brousse
Copy link
Member Author

nicolas-brousse commented Apr 3, 2019

Proposal 2

diff --git a/config/routes.rb b/config/routes.rb
index 7293246..cfa10cc 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 Komponent::Engine.routes.draw do
-  resources :styleguide, only: %i[index show]
+  resources :styleguide, only: %i[index show], module: :komponent
 end
diff --git a/lib/generators/komponent/templates/styleguide/components/sidebar/_komponent_sidebar.html.erb b/lib/generators/komponent/templates/styleguide/components/sidebar/_komponent_sidebar.html.erb
index 195ade3..f6198a5 100644
--- a/lib/generators/komponent/templates/styleguide/components/sidebar/_komponent_sidebar.html.erb
+++ b/lib/generators/komponent/templates/styleguide/components/sidebar/_komponent_sidebar.html.erb
@@ -3,7 +3,7 @@
   <ul class="komponent-sidebar-items">
     <%- components.each do |_k, component| %>
       <li class="komponent-sidebar-item">
-        <%= link_to_unless_current component.title, styleguide_path(id: component.id) %>
+        <%= link_to_unless_current component.title, komponent.styleguide_path(id: component.id) %>
       </li>
     <% end %>
   </ul>
diff --git a/lib/komponent/engine.rb b/lib/komponent/engine.rb
index 477160c..41c40da 100644
--- a/lib/komponent/engine.rb
+++ b/lib/komponent/engine.rb
@@ -9,7 +9,7 @@ require 'komponent/translation'
 
 module Komponent
   class Engine < Rails::Engine
-    isolate_namespace Komponent
+    engine_name "komponent"
 
     rake_tasks do
       load 'komponent/rails/tasks/komponent.rake'

@nicolas-brousse
Copy link
Member Author

Since we don't have helper in app/helpers, I prefer proposal 2. Sounds more clean to me.

I don't think we really needs to isolate our engine.

@@ -5,6 +5,8 @@ class StyleguideController < ::ApplicationController
layout 'komponent'
rescue_from ActionView::MissingTemplate, with: :missing_template

include Rails.application.routes.url_helpers
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure for this. For me this is not a clean way.

@Spone
Copy link
Collaborator

Spone commented Apr 16, 2019

Since we don't have helper in app/helpers, I prefer proposal 2. Sounds more clean to me.

I don't think we really needs to isolate our engine.

I agree. Can you implement solution #2 in your PR?

@nicolas-brousse
Copy link
Member Author

I'll try to found some time in the next days to rework on this.

@nicolas-brousse
Copy link
Member Author

I updated the code to implement proposal 2

@Spone
Copy link
Collaborator

Spone commented Nov 20, 2019

NB: We will need to inform users that they need to replace styleguide_path by komponent.styleguide_path in styleguide/components/sidebar

@nicolas-brousse
Copy link
Member Author

Done

@Spone Spone changed the title WIP: Make application routes usable on example views Make application routes usable on example views Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants