Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 7 years ago

#6275 closed defect (bug) (fixed)

Missing plural on activities for show all comments button

Reported by: danbp's profile danbp Owned by: slaffik's profile slaffik
Milestone: 2.8 Priority: normal
Severity: minor Version:
Component: I18N Keywords: has-patch commit
Cc:

Description

When modifying the default 5 comments
/* Hide long lists of activity comments, only show the latest five root comments. */ in buddypress.js:1898

 if ( jq(this).children('ul').children('li').length < 0 )

and line 1913

if ( i < comment_lis.length - 0 )

to get show comments(x) button as soon as there is 1 comment, the button text brings Show all 1 comments.

the gettext plural is needed in buddypress-functions.php:252

'show_x_comments' => __( 'Show all %d comments', 'buddypress' )

something like

'show_x_comments' => _n( 'Show %d comment', 'Show all %d comments', 'buddypress' )

Thank you. ;-)

Attachments (2)

6275.patch (945 bytes) - added by slaFFik 7 years ago.
6275.png (29.5 KB) - added by slaFFik 7 years ago.

Download all attachments as: .zip

Change History (14)

#1 follow-up: @boonebgorges
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Good catch. Unfortunately, using _n() here is not possible, because we need the singular as well as the plural available when the page renders. In fact, I'm not sure we have a good way of doing this at all at the moment, given that some languages require more than one version of plurals. See https://core.trac.wordpress.org/ticket/22229. I guess, as a bandaid, we could fix this for languages like English and French that have only singular + plural?

#2 in reply to: ↑ 1 @johnjamesjacoby
9 years ago

Replying to boonebgorges:

as a bandaid, we could fix this for languages like English and French that have only singular + plural

I'm fine with this for now; it's better than it being blatantly grammatically incorrect.

#3 @boonebgorges
9 years ago

My point is that we'll remain blatantly grammatically correct in Russian, Japanese, and other widely used languages. But correct in some is, in the absence of a better solution, better than correct in none.

#4 @johnjamesjacoby
9 years ago

I understand your point, and my point is the same as your point, so we are in agreement.

#5 @slaFFik
7 years ago

In my opinion the easiest solution will be instead of this:

'show_all'          => __( 'Show all', 'buddypress' ),
'show_all_comments' => __( 'Show all comments for this thread', 'buddypress' ),
'show_x_comments'   => __( 'Show all %d comments', 'buddypress' ),

to have only 1 record:

'show_all' => __( 'Show all', 'buddypress' ),

As show_all includes all other possible special cases show_all_comments and show_x_comments.

What do you think? Such fix is general and quite easy, and will eliminate this problem.

#6 follow-up: @slaFFik
7 years ago

Other possible solution:

'show_all' => __( 'Show all (%d)', 'buddypress' ),

Which will always display the actual number, if we want end-user give ability to decide whether to click or not based on the number of items.

#7 @slaFFik
7 years ago

  • Component changed from Activity to I18N
  • Milestone changed from Future Release to 2.8

#8 in reply to: ↑ 6 @boonebgorges
7 years ago

Replying to slaFFik:

Other possible solution:

'show_all' => __( 'Show all (%d)', 'buddypress' ),

Which will always display the actual number, if we want end-user give ability to decide whether to click or not based on the number of items.

I like this.

#9 @slaFFik
7 years ago

  • Keywords good-first-bug added

#10 @slaFFik
7 years ago

  • Keywords has-patch added; needs-patch removed
  • Version 2.2.1 deleted

So, after some investigation here is what I've found:

'show_all' => __( 'Show all', 'buddypress' ), - not used at all, be it seems we need to leave it for BC (not sure).

'show_all_comments' => __( 'Show all comments for this thread', 'buddypress' ), - it's a title attribute, so it's good to leave it for accessibility (right, @mercime?)

'show_x_comments' => __( 'Show all %d comments', 'buddypress' ), - here is the source of the problem, when editing buddypress.js to show this show-more link, when there is 1 comment.
The simplest solution: 'show_x_comments' => __( 'Show all comments (%d)', 'buddypress' ),.

Attached image shows how it will look like, and patch - implements this change.

Last edited 7 years ago by slaFFik (previous) (diff)

@slaFFik
7 years ago

@slaFFik
7 years ago

#11 @boonebgorges
7 years ago

  • Keywords commit added; good-first-bug removed
  • Owner set to slaffik
  • Status changed from new to assigned

This looks good.

#12 @slaffik
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11369:

Internationalization: change show all comments button label.

Make 'Show all %d comments' a bit less dependent on the amount of comments, changed to 'Show all comments (%d)'. Thus 1 and 100 comments number will be displayed properly.

Props danbp.
Fixes #6275.

Note: See TracTickets for help on using tickets.