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

Configurable Route Generation #735

Closed
wants to merge 5 commits into from

Conversation

sjaveed
Copy link
Contributor

@sjaveed sjaveed commented Mar 4, 2016

  • friendly_id now takes a routes option which, when set to anything but :friendlyid will generate standard rails style routes when used with e.g. polymorphic_path or form_for etc
  • when the routes option isn't specified, it defaults to the current behavior

Given a model:

class Team < ActiveRecord::Base
  friendly_id :name, use: :slugged
end

it generates:

team_path(team)  # returns /team/123-awesome-team-name
team_path(team.id) # returns /team/123
polymorphic_path(team) # returns /team/123-awesome-team-name

With this patch, given a model like:

class Team < ActiveRecord::Base
  friendly_id :name, use: :slugged, routes: :default
end

it generates:

team_path(team) # should return /team/123
team_path(team.friendly_id) # should return /team/123-awesome-team-name
polymorphic_path(team) # should return /team/123

…ut :friendlyid will generate standard rails style routes when used with e.g. polymorphic_path or form_for etc
@sjaveed
Copy link
Contributor Author

sjaveed commented Mar 4, 2016

@parndt here you go!

@sjaveed sjaveed changed the title Configuration Route Generation Configurable Route Generation Mar 4, 2016
@@ -196,6 +196,7 @@ module Base
def friendly_id(base = nil, options = {}, &block)
yield friendly_id_config if block_given?
friendly_id_config.dependent = options.delete :dependent
# friendly_id_config.routes = (options.delete(:routes) || :friendly)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little confused about this line of code and why it's commented out? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch :-) I think I'd added it in there before I realized I could leverage the set. Commented it out to test the set and then didn't delete it before committing :-) I'll remove this in the next commit.

@parndt
Copy link
Collaborator

parndt commented Mar 4, 2016

@sjaveed that was lightning ⚡ fast.

Could we please have some tests to ensure that we don't regress this behaviour, and a note in the changelog file?

@sjaveed
Copy link
Contributor Author

sjaveed commented Mar 4, 2016

@parndt heh thanks :-) I did say I was frustrated :-) Let me see how to test this sucker properly.

sjaveed added 2 commits March 3, 2016 23:27
@sjaveed
Copy link
Contributor Author

sjaveed commented Mar 4, 2016

@parndt updated to fix an issue with the default value of routes and to add tests. Any specific format you'd like me to follow for the Changelog?

@parndt
Copy link
Collaborator

parndt commented Mar 4, 2016

For changelog just something simple that links to this PR like in https://github.com/norman/friendly_id/blob/master/Changelog.md#520-not-released-yet

@sjaveed
Copy link
Contributor Author

sjaveed commented Mar 4, 2016

@parndt updated the change log as well. Do let me know if you need me to change anything else :-)

@parndt parndt closed this in 8531cdc Mar 5, 2016
@parndt
Copy link
Collaborator

parndt commented Mar 5, 2016

Thanks, I merged it as 8531cdc 😄

@sjaveed
Copy link
Contributor Author

sjaveed commented Mar 5, 2016

Awesome! Thanks @parndt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants