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.

Sign up for GitHub

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

Removed excess /posts/ part from blog-posts resources and fixed localization bug. #463

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

Removed excess /posts/ part from blog-posts resources and fixed localization bug. #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed43652
removed -posts- path from resources
Jul 21, 2016
bd05db1
added /comments suffix to path as an option to show the post
Jul 21, 2016
50a0839
Fixed translation bug in comments/index
Jul 22, 2016
7680b45
Added 301 redirect for old paths for posts
Jul 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/views/refinery/blog/admin/comments/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<% else %>
<p>
<strong>
<%= t('.no_items_yet', :type => action_name.gsub('index', 'new').downcase) %>
<%= t('.no_items_yet', :type => t( (action_name.gsub('index', 'new').downcase), scope: 'refinery.blog.admin.submenu.comments' )) %>
</strong>
</p>
<% end %>
Expand Down
5 changes: 4 additions & 1 deletion config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
Refinery::Core::Engine.routes.draw do
namespace :blog, :path => Refinery::Blog.page_url do
root :to => "posts#index"
resources :posts, :only => [:show]
resources :posts, path: '', :only => [:show] do
Copy link
Member

Choose a reason for hiding this comment

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

That's a breaking change. We should do a redirect from blog/posts/1 to blog/1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? What is the point of this 'posts' level of url? And what redirect are you talking about (what action)?

Copy link
Member

Choose a reason for hiding this comment

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

Was something like:
http://localhost:3000/blog/posts/blog-post-name
Now it is:
http://localhost:3000/blog/blog-post-name

If I've an existing blog indexed on google, all indexed posts will now point to a 404.

I suggest to add a permanent redirect from blog/posts/* to blog/*` in order to keep SEO index fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be
get "posts/:id", to: :show, on: :collection
is better?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I don't know what I was thinking. What's wrong with /blog/posts/:id and /blog/posts ?

Copy link
Member

@bricesanchez bricesanchez Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok if we want to use blog urls without posts level but i would also like to add a redirect to be backward compatible with old urls

Copy link
Contributor Author

@sintro sintro Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@parndt, such url structure (without posts) even appers in this routes set in https://github.com/refinery/refinerycms-blog/blob/master/config/routes.rb#L8 , when you are trying to create the comment to post with invalid fields, you get failed validations and now see the url like 'http://example.com/blog/post-1/comment'. If you try to refresh the page, you will get 404 error. But as for me, this structure is perfect, and 'posts' is unnecessary (even non-friendly) part here, because 'blog' plays the role of 'posts' in this case.
Actually, the same (path: ' ') done in Refinery's extenstions generator, https://github.com/refinery/refinerycms/blob/master/core/lib/generators/refinery/engine/templates/config/routes.rb.erb#L5 , because there is no sense in urls like 'example.com/events/events/1',

get 'comments', on: :member, to: :show
get 'posts/:id(/comments)', on: :collection, to: redirect("#{Refinery::Blog.page_url}/%{id}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way there will be the 301 redirect if someone tries to get the post from old uri

end

get 'feed.rss', :to => 'posts#index', :as => 'rss_feed', :defaults => {:format => "rss"}
get 'categories/:id', :to => 'categories#show', :as => 'category'
Expand Down