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

Session Based BreadcrumbStack #7

Merged
merged 21 commits into from May 18, 2012

Conversation

Projects
None yet
2 participants
Collaborator

58bits commented Apr 7, 2012

No description provided.

@simonc simonc commented on an outdated diff Apr 7, 2012

lib/ariane/rails/controller_helper.rb
@@ -1,15 +1,67 @@
module Ariane
# Adds the ariane helper to controllers.
module ControllerHelper
+
+ # Internal: Sets a hash of cumb levels
+ #
+ # levels - Hash of crumb levels for actions
+ #
+ # Sets a hash of crumb levels for actions that require custom levels
+ #
+ # Returns nothing.
+ def crumb_levels(levels)
@simonc

simonc Apr 7, 2012

Owner

Shouldn't it be called set_crumb_levels or crumb_levels= ?

@58bits 58bits and 1 other commented on an outdated diff Apr 7, 2012

lib/ariane/rails/controller_helper.rb
end
+
+
+
+ # Private: Helper method to retrieve a crumb level from the @crumb_levels hash
+ # if it exists.
+ #
+ # Returns level
+ def get_level(default)
+ return default unless @crumb_levels
+ action = self.action_name.to_sym
+ @crumb_levels[action] || default
+ end
+
+ private :get_level
+
@58bits

58bits Apr 7, 2012

Collaborator

I assigned get level to private explicitly because we don't know what code may follow from our controller_helper.rb - which has been included in application controller. Or can we rely on our module scope and ActionController::Base.send :include, Ariane::ControllerHelper to keep our code separate?

@simonc

simonc Apr 8, 2012

Owner

I was more talking about the way it's written. I rarely see private :method_name.
Generally I see

private

def method_name
  # ...
end
@58bits

58bits Apr 8, 2012

Collaborator

Yes - understood - but private here means that 'all' methods following private will be private, until a public or protected keyword is found. Does ActionController::Base.send :include, Ariane::ControllerHelper and our module namespace mean that this won't affect any other methods in ActionController?

@simonc

simonc Apr 9, 2012

Owner

Yes, the keyword only affects the scope where it is written ;)

@simonc simonc and 1 other commented on an outdated diff Apr 8, 2012

lib/ariane/rails/controller_helper.rb
end
+
+
+
+ # Private: Helper method to retrieve a crumb level from the @crumb_levels hash
+ # if it exists.
+ #
+ # Returns level
+ def get_level(default)
+ return default unless @crumb_levels
+ action = self.action_name.to_sym
+ @crumb_levels[action] || default
+ end
@simonc

simonc Apr 8, 2012

Owner

I don't think this method should take an argument just to return it as default.
The usage should be:

level = get_level || 2

It should not be the work of the method to return the alternative value. It's confusing.

@58bits

58bits Apr 8, 2012

Collaborator

Good catch - and very Rubyesque. ;-)

@simonc simonc and 1 other commented on an outdated diff Apr 8, 2012

lib/ariane/rails/controller_helper.rb
+ Ariane.session = session if Ariane.dynamic_breadcrumb
+ end
+
+ Ariane.breadcrumb
+ end
+
+ # Public: Controller helper method for auto creation of breadcrums.
+ #
+ # Automatically sets the breadcrumb based on controller and action names.
+ #
+ # Returns nothing
+ def auto_set_breadcrumb
+ ariane.add("Home", root_path, 1) if ariane.crumbs.empty?
+
+ if self.action_name == "index"
+ name = self.controller_name.gsub(/_/, " ").capitalize
@simonc

simonc Apr 8, 2012

Owner

Use the following code instead:

name = controller_name.titleize
@58bits

58bits Apr 8, 2012

Collaborator

Okay

@simonc simonc and 1 other commented on an outdated diff Apr 8, 2012

lib/ariane/rails/controller_helper.rb
+ Ariane.breadcrumb
+ end
+
+ # Public: Controller helper method for auto creation of breadcrums.
+ #
+ # Automatically sets the breadcrumb based on controller and action names.
+ #
+ # Returns nothing
+ def auto_set_breadcrumb
+ ariane.add("Home", root_path, 1) if ariane.crumbs.empty?
+
+ if self.action_name == "index"
+ name = self.controller_name.gsub(/_/, " ").capitalize
+ level = get_level(2)
+ else
+ name = self.action_name.capitalize + " " + self.controller_name.singularize.gsub(/_/, " ").capitalize
@simonc

simonc Apr 8, 2012

Owner

same here:

name = "#{action_name.titleize} #{controller_name.titleize}"
@58bits

58bits Apr 8, 2012

Collaborator

Okay ++

@simonc simonc commented on the diff Apr 27, 2012

lib/ariane/rails/controller_helper.rb
+ # Automatically sets the breadcrumb based on controller and action names.
+ #
+ # Returns nothing
+ def auto_set_breadcrumb
+ ariane.add("Home", root_path, 1) if ariane.crumbs.empty?
+
+ if self.action_name == "index"
+ name = controller_name.titleize
+ level = get_level || 2
+ else
+ name = "#{action_name.titleize} #{controller_name.singularize.titleize}"
+ level = get_level || 3
+ end
+
+ ariane.add(name, request.fullpath, level)
+ end
@simonc

simonc Apr 27, 2012

Owner

We still need to figure out how to avoid taking a strong opinion on what the user needs.

@58bits

58bits Apr 28, 2012

Collaborator

Agreed - although the user doesn't have to use auto_set_breadcrumb. For sure we should check to see if root_path is defined - otherwise this will blowup. Apart from that - what if we offer a localization feature - where "Home", and action controller_name are used as the key to a i18n table of values. If they don't exist - then the key is the name. Thoughts?

@simonc

simonc Apr 30, 2012

Owner

Yeah that would be a good idea :)

About the strong opinion, maybe we could check if the controller responds to set_auto_crumb or some method like that and call this code otherwise.
It would allow easy overriding but let the behavior by default. It would have to be documented correctly so it's clear for the users :)

@58bits

58bits May 2, 2012

Collaborator

You mean to have auto_set_breadcrumb called automatically? Not sure - I was hoping the examples in https://github.com/58bits/ariane-sample would be okay - showing how before_filter could be used to control this.

Things suddenly got busy at this end - at least for the next week or two. I think the code here is looking pretty good thanks to your suggestions. Also the dynamic breadcrumb feature does not effect the existing codebase (or gem), since we are now using LevelCrumb and not Crumb. How would you feel about accepting this pull request - for now? And then later we can add the i18n support for crumb labels based on keys.

Thoughts?

@simonc simonc and 1 other commented on an outdated diff Apr 27, 2012

spec/ariane_spec.rb
@@ -49,4 +49,17 @@
Ariane.default_renderer.should == ""
end
end
+
+ describe "#dynamic_breadcrumb" do
+ it "returns the default mode which is false" do
+ Ariane.dynamic_breadcrumb.should == false
@simonc

simonc Apr 27, 2012

Owner

I think it should be

Ariane.dynamic_breadcrumb.should be_false
@58bits

58bits Apr 28, 2012

Collaborator

Done.

@simonc simonc and 1 other commented on an outdated diff Apr 27, 2012

spec/ariane_spec.rb
@@ -49,4 +49,17 @@
Ariane.default_renderer.should == ""
end
end
+
+ describe "#dynamic_breadcrumb" do
+ it "returns the default mode which is false" do
+ Ariane.dynamic_breadcrumb.should == false
+ end
+ end
+
+ describe "#dynamic_breadcrumb=" do
+ it "returns the option set" do
+ Ariane.dynamic_breadcrumb = true
+ Ariane.dynamic_breadcrumb.should == true
@simonc

simonc Apr 27, 2012

Owner
Ariane.dynamic_breadcrumb.should be_true
@58bits

58bits Apr 28, 2012

Collaborator

Done.

@simonc simonc pushed a commit that referenced this pull request May 18, 2012

Simon COURTOIS Merge pull request #7 from 58bits/session
Session Based BreadcrumbStack
de75f34

@simonc simonc merged commit de75f34 into simonc:master May 18, 2012

Owner

simonc commented May 18, 2012

Boom merged !

I bumped the version to 0.4.0.a right before, we still need to work on all this but at least it's in the main repo now !

Thank you :)

Collaborator

58bits commented May 18, 2012

w00t!
On May 18, 2012 9:00 PM, "Simon COURTOIS" <
reply@reply.github.com>
wrote:

Boom merged !

I bumped the version to 0.4.0.a right before, we still need to work on all
this but at least it's in the main repo now !

Thank you :)


Reply to this email directly or view it on GitHub:
#7 (comment)

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