WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#28147 closed defect (bug) (fixed)

No ratings string never used?

Reported by: pavelevap Owned by: SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.9
Component: Themes Keywords: good-first-bug has-patch dev-feedback
Focuses: Cc:

Description

String "No ratings." is probably never used.

https://core.trac.wordpress.org/browser/tags/3.9/src/wp-admin/theme-install.php#L236

There is following condition:

<# if ( data.num_ratings ) { #>

But data.num_ratings is the whole string "(based on %s rating)" and never empty or null. When there is no rating, data.num_ratings will be "(based on 0 rating)". And this is not ideal...

I am not sure which variable contains real number of ratings to patch this issue.

Attachments (6)

28147.diff (556 bytes) - added by valendesigns 6 years ago.
28147.2.diff (516 bytes) - added by kevdotbadger 6 years ago.
28147.3.diff (1.4 KB) - added by kevdotbadger 6 years ago.
Screenshot 2015-01-07 12.52.53.png (515.7 KB) - added by kevdotbadger 6 years ago.
Screenshot 2015-01-07 12.52.50.png (585.2 KB) - added by kevdotbadger 6 years ago.
28147.4.diff (1.4 KB) - added by valendesigns 6 years ago.

Download all attachments as: .zip

Change History (14)

#1 @jorbin
6 years ago

  • Component changed from I18N to Themes
  • Keywords good-first-bug added

wp_ajax_query_themes will need to be modified to include the raw count of ratings and we will need to update the check to look for that.

#2 @SergeyBiryukov
6 years ago

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

@valendesigns
6 years ago

#3 @valendesigns
6 years ago

  • Keywords has-patch added; needs-patch removed

There's a simpler solution. I've added patch 28147.diff that fixes this issue without any major changes.

@kevdotbadger
6 years ago

#4 @kevdotbadger
6 years ago

Actually, i think there's an even easier way, which is what I think the original developer intended.

data.num_ratings is always a string, which is the obvious problem. jorbin mentioned amending some code to return the raw count, this actually exists in data.rating (as an int), this is what should be used.

28147.2.diff fixes this.

@kevdotbadger
6 years ago

#5 @kevdotbadger
6 years ago

  • Keywords dev-feedback added

This actually got me thinking. Should the star's actually show if there's no rating? They are always going to blank, and it doesn't really add anything to the page. In fact, in my opinion, it looks negative as you could easily interpret the theme as rubbish purely because there's no rating.

28147.3.diff hides the stars and says 'This theme hasn't been rated' if there's no rating, and does the default if there is a rating (stars, then '(based on X ratings'). See the screenshots.

Anyone disagree or have a better idea?

Last edited 6 years ago by kevdotbadger (previous) (diff)

#6 @SergeyBiryukov
6 years ago

28147.3.diff looks good to me. (A small note, we don't generally escape apostrophes with slashes like that, the preferred way is to switch to double quotes, see the coding standards.)

I'd suggest changing the string to "This theme has not been rated yet".

Last edited 6 years ago by SergeyBiryukov (previous) (diff)

@valendesigns
6 years ago

#7 @valendesigns
6 years ago

The refreshed 28147.4.diff patch does not use contracted english grammar and is created from the repository root instead of inside src.

#8 @SergeyBiryukov
6 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 31072:

Theme install: Display an appropriate string if theme has not been rated yet.

props kevdotbadger, valendesigns.
fixes #28147.

Note: See TracTickets for help on using tickets.