add taging ability to pluralize #5490

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
Contributor

lellisga commented Mar 17, 2012

the pluralize can now return a tagged count. This allow us to easily manipulate the count number with jQuery and CSS

@lest lest and 1 other commented on an outdated diff Mar 17, 2012

actionpack/lib/action_view/helpers/text_helper.rb
@@ -412,6 +419,12 @@ def set_cycle(name, cycle_object)
@_cycles = Hash.new unless defined?(@_cycles)
@_cycles[name] = cycle_object
end
+
+ # The pluralize helper need to tag the count number
+ # between two html tags.
+ def tag_count(tag,count)
+ tag == nil ? count : "<#{tag}>#{count}</#{tag}>"
@lest

lest Mar 17, 2012

Contributor

I think that it's better to use content_tag(tag, count) instead of string interpolation.

@sarcilav

sarcilav Mar 17, 2012

Yeah, I agree with @lest , because with content_tag it will allow the use of classes and other html attributes

I think the api gets a bit cumbersome accepting 5 arguments, but I'm unsure about better ways now. An idea would be to accept a block, something like:

pluralize(1, 'foo') { |count| content_tag(:span, count) }

Another option is to create a separate method for that specific case. The method is so simple though, that I'm unsure about both approaches.

@lellisga from the @carlosantoniodasilva 's comments I figured out that what about if I want to add html_options or a tag to the pluralized word.
So to let it works properly, for me looks like a better option to accept a block like pluralize(0, 'bar') { | count , word | ... } that let you do whatever you want

Contributor

lellisga commented Mar 18, 2012

Wow! Thanks @carlosantoniodasilva and @sarcilav. I was thinking in tagging the word too but I then realize that the method will have to accept a lot more arguments and the word can still be accessed using CSS or JQ. I'll go ahead a do it with the block (have to read a little bit about that :D). Thanks again!

Contributor

lellisga commented Mar 19, 2012

@carlosantoniodasilva and @sarcilav . I really don't know how good this can be put it works! https://gist.github.com/2088406

@lellisga sounds good :). I just don't think you need to check for block arity. If a block is given, yield both count and word and let the user do the work, otherwise use the current logic. This way you wouldn't need the block argument anymore.

Contributor

lellisga commented Mar 19, 2012

sorry, didn';t see the comment :(

Contributor

lellisga commented Mar 19, 2012

@carlosantoniodasilva but I still have to ask in order to pluralize the word

Owner

rafaelfranca commented Mar 19, 2012

Hey @lellisga I agree with @carlosantoniodasilva. You don't need to check the block arity. If the user need to customize the count but not the word he can do something like this:

pluralize(3, 'person', 'users') { |count, word| "#{content_tag(:b, count)} #{word}"}

@rafaelfranca rafaelfranca commented on an outdated diff Mar 19, 2012

actionpack/lib/action_view/helpers/text_helper.rb
@@ -189,8 +190,19 @@ def excerpt(text, phrase, *args)
#
# pluralize(0, 'person')
# # => 0 people
- def pluralize(count, singular, plural = nil)
- "#{count || 0} " + ((count == 1 || count =~ /^1(\.0+)?$/) ? singular : (plural || singular.pluralize))
+ #
+ # pluralize(1, 'person'){|count| content_tag(:em,count) }}
+ # # => <em>1</em> person
+ #
+ # pluralize(3, 'person', 'users'){|count, word| "#{content_tag(:b,count)} #{content_tag(:span,word)}"}
+ # # => <b>3</b> <span>users</span>
+ def pluralize(count, singular, plural = nil,&block)
+ word = ((count == 1 || count =~ /^1(\.0+)?$/) ? singular : (plural || singular.pluralize))
+ if block_given?
+ block.arity == 2 ? "#{yield(count,word)}" : "#{yield(count,word)} #{word}"
@rafaelfranca

rafaelfranca Mar 19, 2012

Owner

Some considerations about code style. Put one space after the ,. Like this:

yield(count, word)

@rafaelfranca rafaelfranca commented on an outdated diff Mar 19, 2012

actionpack/lib/action_view/helpers/text_helper.rb
@@ -189,8 +190,19 @@ def excerpt(text, phrase, *args)
#
# pluralize(0, 'person')
# # => 0 people
- def pluralize(count, singular, plural = nil)
- "#{count || 0} " + ((count == 1 || count =~ /^1(\.0+)?$/) ? singular : (plural || singular.pluralize))
+ #
+ # pluralize(1, 'person'){|count| content_tag(:em,count) }}
+ # # => <em>1</em> person
+ #
+ # pluralize(3, 'person', 'users'){|count, word| "#{content_tag(:b,count)} #{content_tag(:span,word)}"}
@rafaelfranca

rafaelfranca Mar 19, 2012

Owner

Put a space before the { when it is a block, after it and before the }. Like this:

pluralize(3, 'person', 'users'){ |count, word| "#{content_tag(:b, count)} #{content_tag(:span, word)}" }

@rafaelfranca rafaelfranca commented on an outdated diff Mar 19, 2012

actionpack/test/template/text_helper_test.rb
@@ -256,6 +256,9 @@ def test_pluralization
assert_equal("10 buffaloes", pluralize(10, "buffalo"))
assert_equal("1 berry", pluralize(1, "berry"))
assert_equal("12 berries", pluralize(12, "berry"))
+ assert_equal("<b>1</b> count", pluralize(1,"count") { |count| content_tag(:b,count) })
+ assert_equal("<span>2</span> counts", pluralize(2,"count"){ |count| content_tag(:span,count) })
+ assert_equal("<p>1</p> <span>count</span>", pluralize(1,"count") {|count,word,| "#{content_tag(:p,count)} #{content_tag(:span,word)}" })
@rafaelfranca

rafaelfranca Mar 19, 2012

Owner

The tests should follow the same code style too. Also this test had one more , after the word block argument.

Contributor

lellisga commented Mar 19, 2012

@rafaelfranca Thanks a lot!

@lellisga hey, sounds great. I'd just say you don't need to use string interpolation here: "#{yield(count, word)}", you'd be fine with just the yield call: yield(count, word). Other than that, I think you just need to squash everything on a single commit :). Thanks.

@rafaelfranca rafaelfranca commented on an outdated diff Mar 19, 2012

actionpack/lib/action_view/helpers/text_helper.rb
@@ -189,8 +190,15 @@ def excerpt(text, phrase, *args)
#
# pluralize(0, 'person')
# # => 0 people
+ #
+ # pluralize(1, 'person'){ |count, word| "#{content_tag(:em, count)} #{word} }
+ # # => <em>1</em> person
+ #
+ # pluralize(3, 'person', 'users'){ |count, word| "#{content_tag(:b, count)} #{content_tag(:span,word)}" }
+ # # => <b>3</b> <span>users</span>
@rafaelfranca

rafaelfranca Mar 19, 2012

Owner

And in this line you still have content_tag(:span,word) without the space after the ,

Contributor

lellisga commented Mar 19, 2012

@carlosantoniodasilva done!, thanks again

Owner

rafaelfranca commented Mar 19, 2012

@lellisga thank you!

Contributor

lellisga commented Mar 19, 2012

@rafaelfranca Thank to you

Owner

pixeltrix commented Mar 19, 2012

@lellisga how does wrapping the count make it easier to update via jQuery - you still might need to changed the word? For example if you go from '1 item' to '2 items'.

Contributor

lellisga commented Mar 20, 2012

@pixeltrix Sure but if that's the case is much cheaper to get the number and do your logic instead of doing a new request, that's not all, having both tagged makes it easier to jquery to find the count or the word, without the tag it will get the complete phrase thus you'll have to do regex to get the count or the word

Contributor

lellisga commented Apr 28, 2012

any word on this?

Owner

pixeltrix commented Apr 28, 2012

Sorry for not getting back sooner. We discussed it in the Campfire room and the consensus was that it was an application level concern - thanks for your contribution though.

pixeltrix closed this Apr 28, 2012

Contributor

lellisga commented Apr 28, 2012

(Y) thanks to u

Contributor

graysonwright commented Feb 9, 2013

Could we revisit this topic?

This seems like a very light commit, with only one SLOC changed, and I believe that it fixes functionality that is now very limited.

The pluralize method takes a number and a string, and returns only a string -- once you call the method, you constrain yourself to working with a string from that point forward. If you want to further manipulate the word or the number, the method becomes useless to you.

The use case of tagging the number and word separately came up. For example, if you want to manipulate the number with styling, as in this image:
tagged_numbers

something like the following must currently be done:

<span class='number-emph'>
  <%= @user.projects.count %>
</span><br/>
<%= @user.projects.one? 'Project' : 'Projects' %>

The last line -- checking to see if the count is one, then selecting a version of a word to match, is almost exactly what's being done in the pluralize method already -- it makes sense to expand the method to include this use case.

Manipulating the page with javascript, or scraping pages for data is also made a lot easier by being able to tag the word and number separately.

The method becomes much more effective and powerful when you have the option of simply transforming the word to match the count, instead of being forced to concatenate the two into a string.

Contributor

dalibor commented Feb 9, 2013

@graysonwright you can just create a helper method that wraps pluralize + html tag.

Contributor

graysonwright commented Feb 12, 2013

Yeah, I suppose that would work. It just seems like it's really close to what the pluralize method is already doing.

I guess I'm confused about the criteria for accepting the feature into Rails. @pixeltrix mentioned that it was an application-level concern, but it seems to me like it's a natural extension of the pluralize helper that's already in Rails. What's the criteria for making that decision?

Thanks for your help.

Contributor

dalibor commented Feb 12, 2013

@graysonwright I'm not sure if there is an exact criteria, but my opinion is that the framework should do the hard stuff for you, and leave the easy stuff to you.

Other thing for this specific case is that pluralize method should do only one thing - pluralization. If I want html tag wrapping I would introduce another method like pluralize_with_wrap. But since it's so easy to write that helper myself, it's best to prevent the framework from getting bloated and keep it easy to maintain. ;)

Owner

pixeltrix commented Feb 12, 2013

@graysonwright there are basically two criteria for whether a new feature is added to Rails:

  1. Can it be used within the Rails codebase? This covers most of Active Support which is not a general Ruby extension library like Ruby Facets, but specifically for Rails or developing web applications.
  2. Is it a generalized feature and be of use to a large proportion of Rails users? For example we have a generic <video> tag helper but not a helper for generating embedded YouTube videos even though it might be used by a large number of developers.
Contributor

graysonwright commented Feb 12, 2013

Makes sense.

I appreciate the explanation. Thanks!

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