Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

HTML formatting to Rails::InfoController#routes #8521

Merged
merged 1 commit into from Dec 17, 2012

Conversation

Projects
None yet
8 participants
Member

schneems commented Dec 15, 2012

This PR adds formatting and meta-data to the display of the internal routes. Users can now toggle between showing helpers with the _path or _url suffix.

There are multiple ways to achieve this, this method uses partials for formatting and meta-data. The partials can be re-used when rendering routing_error.erb, though that will need to be in a separate PR.

ATP Railties

Contributor

gaurish commented Dec 15, 2012

Looks good, Minor nitpicking though

  • the markup is Missing <thead> & <tbody> tags - Those table elements are basically entered by the browser if not present, but it's better to put them in to make the markup standards compliant.

I wonder, if some basic styling(zebra strips, borders, headers, numbering etc) which would make it more readable specially with long routes would be a good idea?

Member

steveklabnik commented Dec 15, 2012

I think this alleviates @dhh's concerns, except that it's not an Engine. That said, if we're forcing this to be one, that involves a lot more change than this.

👍 from me.

Owner

dhh commented Dec 15, 2012

Hmm, I don't think it's going to fly to have Rails error pages depend on jquery. Can we rewrite this in vanilla JS and not require the inclusion of that monster lib just to display an error page?

Owner

dhh commented Dec 15, 2012

(Besides that though this is much nicer! Thanks for working on it).

Member

schneems commented Dec 15, 2012

Thanks, I can add the table tag and rework the JS to remove jQuery.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Dec 16, 2012

railties/lib/rails/templates/rails/info/routes.html.erb
@@ -1,3 +1,8 @@
+<style>
+ .route-row td {padding: 0 30px;}
@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2012

Owner

How about .routeTable td?

@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2012

Owner

Or actually, you can use the id #routeTable.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Dec 16, 2012

railties/lib/rails/templates/rails/info/routes.html.erb
+<%= render layout: "rails/info/routes_wrapper" do %>
+ <%= render partial: "rails/info/route", collection: @routes %>
+<% end %>
@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2012

Owner

I think it might be better to not depend on render :layout, since the template is so small and there's no reuse, we could just add the layout content here. Wdyt?

@schneems

schneems Dec 16, 2012

Member

There isn't reuse- yet, but after this PR goes through i'm planning on using the layout again in the routing exception page. That way we are consistent every time we show routes in the browser. Thats also why I re-factored the JS to not rely on jQuery.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Dec 16, 2012

railties/lib/rails/templates/rails/info/routes.html.erb
+<%= render layout: "rails/info/routes_wrapper" do %>
+ <%= render partial: "rails/info/route", collection: @routes %>
@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2012

Owner

render partial: 'route' should work.

@schneems

schneems Dec 16, 2012

Member

With partial rendering I prefer to be explicit, so that if need be someone could copy-pasta that line and it would still work, also no one is left wondering exactly which _route partial it is rendering. While it is not a big deal in small projects, it gets much more convoluted as it grows. This way the same code would be present in the error render page as in this page. Again, this is just personal preference, always, i'll be happy to switch though if you prefer the shorter option.

@carlosantoniodasilva

carlosantoniodasilva Dec 16, 2012

Owner

Well, my personal preference is for the other option, but there's no need to change just because of my preference :)

Awesome @schneems, thanks for working on that 👍

Member

schneems commented Dec 16, 2012

@dhh I updated the PR to only use vanilla JS. Went from 7 lines of jQuery to 30 of JS. I'm open to criticism/feedback on the JS implementation, I usually work with jQuery/coffee-script so it's likely i'm doing something inefficiently. There are still other comments on the PR that need to be addressed, but I wanted to push the JS changes and get feedback.

Owner

dhh commented Dec 16, 2012

Good deal on the JS. Rather another 23 lines by hand than the thousands of lines that jquery would have added. 👍

Member

schneems commented Dec 16, 2012

Updated to use proper thead and tbody tags. Updated style to use id's instead of classes.

Member

schneems commented Dec 17, 2012

Updated to use var in JS when setting variables. Anything else that needs work?

Owner

dhh commented Dec 17, 2012

It's fine as it is but I would actually inline all the partials into the one info template.

@pixeltrix pixeltrix and 4 others commented on an outdated diff Dec 17, 2012

...b/rails/templates/rails/info/_routes_wrapper.html.erb
@@ -0,0 +1,57 @@
+<style type='text/css'>
+ #routeTable td {padding: 0 30px;}
+ #routeTable {margin: 0 auto 0;}
+</style>
+
+<table id='routeTable' class='routeTable'>
@pixeltrix

pixeltrix Dec 17, 2012

Owner

Are these class & id names in line with Rails guidelines - I remember we switched the formatting of fieldWithErrors to field_with_errors

@rafaelfranca

rafaelfranca Dec 17, 2012

Owner

Yes, also there are classes using dash too. We need to reach a standard. I think the underscore version is preferred.

@steveklabnik

steveklabnik Dec 17, 2012

Member

I'm pretty sure CSS people use dashes. Not 100% on this. @chriseppstein?

@gaurish

gaurish Dec 17, 2012

Contributor

Class & ID names are usually dashes(class="table-stripped") & sometimes camelcased(class="tableStripped") too. But seldom underscores(class="table_stripped") are used.

I would suggest, choosing one & sticking with it as doesn't matter much as song as we are consistent.

@rafaelfranca

rafaelfranca Dec 17, 2012

Owner

I know they use dashes, but all the Rails helpers generate with underscore, so is better to use what Rails use.

@dhh

dhh Dec 17, 2012

Owner

👍 on sticking to Rails defaults with underscores.

On Dec 17, 2012, at 4:32 PM, Rafael Mendonça França wrote:

In railties/lib/rails/templates/rails/info/_routes_wrapper.html.erb:

@@ -0,0 +1,57 @@
+<style type='text/css'>

  • #routeTable td {padding: 0 30px;}
  • #routeTable {margin: 0 auto 0;}
    +</style>

+


I know they use dashes, but all the Rails helpers generate with underscore, so is better to use what Rails use.


Reply to this email directly or view it on GitHub.

@acapilleri acapilleri commented on an outdated diff Dec 17, 2012

...b/rails/templates/rails/info/_routes_wrapper.html.erb
@@ -0,0 +1,57 @@
+<style type='text/css'>
+ #routeTable td {padding: 0 30px;}
+ #routeTable {margin: 0 auto 0;}
+</style>
+
+<table id='routeTable' class='routeTable'>
+ <thead>
+ <tr>
+ <th>Helper<br />
+ <%= link_to "Path", "#", 'data-route-helper' => '_path',
@acapilleri

acapilleri Dec 17, 2012

Contributor

why not something like:
<%= link_to "Path", "#", data: {'route-helper': :_path} .......

Member

schneems commented Dec 17, 2012

updated to have consistent id and class names with underscores, and all partials have been removed in favor of having all the code in the template.

@acapilleri acapilleri commented on the diff Dec 17, 2012

railties/lib/rails/templates/rails/info/routes.html.erb
+<style type='text/css'>
+ #route_table td {padding: 0 30px;}
+ #route_table {margin: 0 auto 0;}
+</style>
+
+<table id='route_table' class='route_table'>
+ <thead>
+ <tr>
+ <th>Helper<br />
+ <%= link_to "Path", "#", 'data-route-helper' => '_path',
@acapilleri

acapilleri Dec 17, 2012

Contributor

Why do you not use the new hash syntax on link_to?

<%= link_to "Path", "#", data: { 'route-helper': :_path} ,...

@schneems

schneems Dec 17, 2012

Member

For starters, that causes an exception as it isn't valid ruby. We also only have one data attribute, so it seems a bit overkill to use a hash when we don't need one.

@acapilleri

acapilleri Dec 17, 2012

Contributor

yes you're ritght , also I don't know that If the symbol or string of the key contains special characters such as '-', I have to use the Ruby 1.8 syntax

@schneems

schneems Dec 17, 2012

Member

Thanks for looking over the code and commenting :) Happy to clear up any confusion, generally more eyes on more code is only a good thing.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Dec 17, 2012

railties/lib/rails/templates/rails/info/routes.html.erb
+ title: "Returns a relative path (without the http or domain)" %> /
+ <%= link_to "Url", "#", 'data-route-helper' => '_url',
+ title: "Returns an absolute url (with the http and domain)" %>
+ </th>
+ <th>HTTP Verb</th>
+ <th>Path</th>
+ <th>Controller#Action</th>
+ </tr>
+ </thead>
+ <tbody>
+ <% @routes.each do |route| %>
+ <tr class='route_row' data-helper='path'>
+ <td data-route-name='<%= route[:name] %>'>
+ <% if route[:name].present? %>
+ <%= "#{route[:name]}<span class='helper'>_path</span>".html_safe %>
@rafaelfranca

rafaelfranca Dec 17, 2012

Owner

I don't think we need this string interpolation here.

<%= route[:name] %>
<span class='helper'>_path</span>
@schneems

schneems Dec 17, 2012

Member

you can't have them on separate lines or there will be whitespace between the name and the _path

Member

schneems commented Dec 17, 2012

updated to remove string interpolation

HTML formatting to Rails::InfoController#routes
This PR adds formatting and meta-data to the display of the internal routes. Users can now toggle between showing helpers with the `_path` or _`url` suffix. 

There are multiple ways to achieve this, this method uses partials for formatting and meta-data. The partials can be re-used when rendering `routing_error.erb`, though that will need to be in a separate PR.

![](http://f.cl.ly/items/3A2p3c1T1t2f2X2R2K2S/Screen%20Shot%202012-12-12%20at%202.28.01%20PM.png)


ATP Railties

rafaelfranca added a commit that referenced this pull request Dec 17, 2012

Merge pull request #8521 from schneems/schneems/html-routes
HTML formatting to Rails::InfoController#routes

@rafaelfranca rafaelfranca merged commit 75ba92e into rails:master Dec 17, 2012

schneems added a commit to schneems/rails that referenced this pull request Dec 17, 2012

Format routes as html on debug page
When someone gets a routing exception, the routes are rendered (starting in Rails 4.0). This PR brings parity between the html routes in the `rails/info/routes` path and when rendered from an exception. This is the continuation of #8521 which brought html formatted routes. 

In addition to bringing parity to the two views, we're keeping our views DRY by rendering off of the same partials. In this case Railties depends on partials provided by ActionDispatch. I'm open to alternative implementations. Ideally both views will use the same code so any improvements or updates to it will be reproduced on both.

<hr />

![](http://f.cl.ly/items/3O1D0K1v0j0i343O3T3T/Screen%20Shot%202012-12-17%20at%203.07.20%20PM.png)

@schneems schneems referenced this pull request in schneems/sextant Dec 20, 2012

Closed

Update lib/rails/routes.rb #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment