WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#21479 closed defect (bug) (wontfix)

Twenty Twelve: hide category and tag output in post meta if only 1 term

Reported by: lancewillett Owned by: lancewillett
Milestone: Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Props ryanimel and iandstewart for the report and idea for fix.

First patch attached.

Attachments (6)

21479.patch (2.2 KB) - added by lancewillett 2 years ago.
Patch from ryanimel
21479.2.patch (1.4 KB) - added by jkudish 2 years ago.
21479.1.patch (2.3 KB) - added by iandstewart 2 years ago.
Modifying ryanimel's patch to flush out the cool cats when a user delete's categories too (and props to ryanimel for catching that right away)
21479.3.patch (1.1 KB) - added by lancewillett 2 years ago.
Simpler check and correct logic for 0 or 1 category existing
21479.4.patch (1.3 KB) - added by SergeyBiryukov 2 years ago.
21479.5.diff (1.4 KB) - added by obenland 2 years ago.

Download all attachments as: .zip

Change History (26)

lancewillett2 years ago

Patch from ryanimel

comment:1 ryanimel2 years ago

  • Cc ryan@… added

jkudish2 years ago

comment:2 jkudish2 years ago

  • Cc joachim.kudish@… added

In 21479.2.patch, I've simplified and cleaned up the function in several ways:

  • I don't think it helps anything to cache the function in a transient as get_categories calls get_terms which has caching built-in
  • Renamed the function to 'twentytwelve_is_categorized_site' to be a bit more descriptive about what it does
  • Renamed the variable $all_the_cool_cats to $non_empty_categories to be a bit more descriptive about it is
  • Simplified the if statements

iandstewart2 years ago

Modifying ryanimel's patch to flush out the cool cats when a user delete's categories too (and props to ryanimel for catching that right away)

comment:3 iandstewart2 years ago

jkudish FTW. Much simpler.

comment:4 jkudish2 years ago

  • Keywords commit added

comment:5 lancewillett2 years ago

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

In [21444]:

Twenty Twelve: hide category and tag output in post meta if only 1 term. Props ryanimel, jkudish and fixes #21479.

comment:6 lancewillett2 years ago

In [21449]:

Twenty Twelve: check for empty variable for simpler logic, props jkudish. See #21479.

comment:7 lancewillett2 years ago

In [21489]:

Twenty Twelve: make twentytwelve_is_categorized_site() pluggable, see #21479.

comment:8 nacin2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This looks like it would be running extra queries on the frontend. We should be sure they are necessary.

comment:9 follow-up: nacin2 years ago

! empty( $categories_list ) can simply be $categories_list. Also, count() is unnecessary when empty() is already there.

comment:10 in reply to: ↑ 9 lancewillett2 years ago

Replying to nacin:

! empty( $categories_list ) can simply be $categories_list.

Agreed.

Also, count() is unnecessary when empty() is already there.

The check is supposed to return false if only 1 category exists. So, the check should be count( $non_empty_categories ) < 2 instead, right?

lancewillett2 years ago

Simpler check and correct logic for 0 or 1 category existing

SergeyBiryukov2 years ago

comment:11 follow-up: SergeyBiryukov2 years ago

We could use wp_count_terms(), which performs a faster query (SELECT COUNT(*) instead of requesting all the category data, 0.8ms vs. 4.0ms on my install).

21479.4.patch also simplifies the $tag_list check.

obenland2 years ago

comment:12 obenland2 years ago

21479.5.diff builds on @SergeyBiryukov's patch, further simplifying the is_categorized() check.

comment:13 in reply to: ↑ 11 lancewillett2 years ago

  • Keywords commit removed

Replying to SergeyBiryukov:

We could use wp_count_terms(), which performs a faster query (SELECT COUNT(*) instead of requesting all the category data, 0.8ms vs. 4.0ms on my install).

Nice. :)

comment:14 follow-up: lancewillett2 years ago

Ryan and Ian — can you weigh in on why you think this UX improvement is worth adding extra queries to each page load?

Discussing today in #wordpress-dev IRC Nacin is voting to remove this and "just live with Uncategorized."

comment:15 lancewillett23 months ago

If no votes for why the UX is important for the default theme to hide "Uncategorized" from display, we'll revert and punt this to a better fix on the core side, meaning only 1 category and it's named Uncategorized will be the same as having 0 categories on the front end.

Better to fix it in core for all themes than just one theme here.

comment:16 in reply to: ↑ 14 iandstewart23 months ago

Replying to lancewillett:

can you weigh in on why you think this UX improvement is worth adding extra queries to each page load?

It doesn't make sense to display only 1 category if every post is in that category. One category is no categories, imo. I think it's confusing for new users and worse, just plain ugly if that category is uncategorized -- though I'd feel the same way no matter what that one category was named. It's also annoying for users using WordPress as a small CMS for brochure sites where they may not be using any categories at all. We do it with tags, we should do it with categories.

+1 for this being in core and available for every theme -- with a new template tag. Themes need to be able to make markup for category lists conditional on there actually being categories to theme.

comment:17 ryanimel23 months ago

Better in core than a theme, absolutely. It would be nice if the theme could have it in, just for the sake of working that way sooner, but I understand not wanting to add extra queries. +1 for core too.

comment:18 lancewillett23 months ago

In [21543]:

Twenty Twelve: remove twentytwelve_is_categorized_site() in favor of a theme-agnostic future core fix for this issue, see #21479.

Also simplify the logic for checking tag and categories, props SergeyBiryukov and obenland.

comment:19 lancewillett23 months ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

See #14901 for a possibly related discussion for hiding Uncategorized in front-end display.

Closing this as the solution should not be in the theme itself.

comment:20 SergeyBiryukov23 months ago

  • Milestone 3.5 deleted
Note: See TracTickets for help on using tickets.