WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 6 months ago

#4969 assigned enhancement

Make wp_list_* functions all behave similarly...

Reported by: pishmishy Owned by: igmoweb
Milestone: Future Release Priority: normal
Severity: normal Version: 2.3
Component: Themes Keywords: good-first-bug has-patch
Focuses: template Cc:

Description

The sidebar template typically uses lists of lists to prettily display lists of pages, categories and bookmarks. Lists of pages, categories and bookmarks are forced to be items of lists themselves as the functions wp_list_pages(),wp_list_categories() and wp_list_bookmarks() wrap the output of functions in <li>..</li> tags by default.

Lists are intended to indicate something about the structure of the document where as these forced <li> tags appear to be intended to provide pretty indentation. Nested-lists can also provide confusion for non-visual readers.

wp_list_bookmarks() provides the ability to override this with category_before and category_after options. wp_list_pages() and wp_list_categories() should offer similar options. Ideally the accessible approach would be the default for these options but I don't believe this is possible without breaking existing themes.

Attachments (8)

4969-list-cats.diff (1.1 KB) - added by Otto42 9 years ago.
Patch for wp_list_categories
4969-list-pages.diff (1.2 KB) - added by Otto42 9 years ago.
Patch for wp_list_pages
4969.patch (1.9 KB) - added by pishmishy 9 years ago.
Revised patch
4969.diff (969 bytes) - added by wojtek.szkutnik 6 years ago.
4969.2.patch (3.2 KB) - added by igmoweb 11 months ago.
4969.3.diff (3.5 KB) - added by igmoweb 11 months ago.
Patch reviewed
4969.3.tests.diff (3.1 KB) - added by igmoweb 11 months ago.
Unit tests
4969.4.patch (6.6 KB) - added by igmoweb 6 months ago.
Changes + Unit tests

Download all attachments as: .zip

Change History (35)

#1 follow-up: @pishmishy
9 years ago

  • Keywords has-patch lists added
  • Owner changed from anonymous to pishmishy
  • Severity changed from trivial to normal
  • Status changed from new to assigned

Added patch. I'm not sure why the options are called category_before, category_after. I suspect they need to be changed to something more descriptive but I've continued the pattern for now.

#2 in reply to: ↑ description ; follow-up: @Otto42
9 years ago

Replying to pishmishy:

The sidebar template typically uses lists of lists to prettily display lists of pages, categories and bookmarks. Lists of pages, categories and bookmarks are forced to be items of lists themselves as the functions wp_list_pages(),wp_list_categories() and wp_list_bookmarks() wrap the output of functions in <li>..</li> tags by default.

Lists are intended to indicate something about the structure of the document where as these forced <li> tags appear to be intended to provide pretty indentation. Nested-lists can also provide confusion for non-visual readers.

The purpose of these lists is not for indentation, it's for document structure.

Generally speaking, the entire sidebar can be considered one big list of items. Each "item" is a whole sidebar entry. This is a perfectly acceptable structure, you have a big list of entries. The fact that each entry is large vertically does not make the unordered list structure incorrect. And indentation is not a factor here either, generally speaking, as the entire sidebar is styled to a specific position.

Now, the individual elements of that sidebar might contain list information themselves, such as a list of pages or list of bookmarks or what have you. But the content of that sidebar list item is irrelevant, it can be anything desired.

wp_list_bookmarks() provides the ability to override this with category_before and category_after options.

wp_list_bookmarks is a special case, as it generates *multiple* list items intended for inclusion into a sidebar. By default, each category becomes a separate sidebar entry, instead of the entire unit being one sidebar entry with multiple subentries.

Because of this multiple nature, it needs these extra before and after options because not all themes use lists for the entire sidebar. I've seen sidebars that use nested divs, for example. These themes would have a div around each sidebar section, and so the wp_list_bookmarks would need to know those div elements in order to wrap each one in a div instead of a list.

wp_list_pages() and wp_list_categories() should offer similar options.

wp_list_pages and wp_list_categories do not actually generate multiple sidebar, and so they do not need these similar options.

Recommend closing this as wontfix.

#3 in reply to: ↑ 1 @Otto42
9 years ago

Replying to pishmishy:

Added patch. I'm not sure why the options are called category_before, category_after. I suspect they need to be changed to something more descriptive but I've continued the pattern for now.

wp_list_bookmarks() uses category before and after because it's wrapping each category in a separate sidebar entry. Use of this pattern in these other functions is incorrect, even if it was necessary.

#4 @Otto42
9 years ago

On the other hand, looking closer, I do see your point to some degree. As far as consistency goes, it would make a lot more sense if the pages and categories functions had title_before and after as well as a normal before and after wrapping the whole thing. The style == 'list' is being misused in the categories, for example.

Here's a patch for wp_list_categories that I think is more consistent overall.

@Otto42
9 years ago

Patch for wp_list_categories

@Otto42
9 years ago

Patch for wp_list_pages

#5 @Otto42
9 years ago

  • Summary changed from Lists within lists to Make wp_list_* functions all behave similarly...

wp_list_pages is trickier, it doesn't make much sense to not have it in an unordered list internal to the sidebar item, since it's hierarchical in nature. So it was strangely overriding the existence of a title_li to make it not display the UL even though it had no way to not display the LIs around each entry.

Patch attached to make it more consistent with the others.

#6 in reply to: ↑ 2 ; follow-up: @pishmishy
9 years ago

Replying to Otto42:

The purpose of these lists is not for indentation, it's for document structure.

Generally speaking, the entire sidebar can be considered one big list of items.

That's fine if it's what you're after (and I admit for most people it is), but it'd be nice if a theme author could have option of not using a list of lists if they so desire. Allow consistant unwrapping of the <li> tags if desired doesn't come at any cost.

Many thanks for your work on further patches for this ticket.

#7 in reply to: ↑ 6 ; follow-up: @Otto42
9 years ago

Replying to pishmishy:

That's fine if it's what you're after (and I admit for most people it is), but it'd be nice if a theme author could have option of not using a list of lists if they so desire.

I don't understand your objection, theme authors do have that option. Many themes use divs instead, for example. All the bits in there where it generates lists for sidebar items can be overriden with arguments.

#8 @pishmishy
9 years ago

  • Keywords needs-testing added

I haven't looked at this ticket in a while. I'm still convinced that we should be able to use _before and _after arguments with this function.

#9 @pishmishy
9 years ago

  • Milestone changed from 2.5 to 2.6

Bumping milestone for feature freeze.

#10 in reply to: ↑ 7 @vlogtheimpaler
9 years ago

Replying to Otto42:

Replying to pishmishy:

That's fine if it's what you're after (and I admit for most people it is), but it'd be nice if a theme author could have option of not using a list of lists if they so desire.

I don't understand your objection, theme authors do have that option. Many themes use divs instead, for example. All the bits in there where it generates lists for sidebar items can be overriden with arguments.

I believe the objection presented there is allowing theme authors to be able to retain categorization while broken out of list mode. While currently it's possible to replace the tags for list items, the unordered list element is still present when categorization is enabled. Meaning if you want to keep categories but drop lists, in it's current form it's a bit inflexible to do so.

A bug also worth mentioning, is while attempting to pass double quotes as part of your string in either the before or after portions for tags that write lists. Like wp_list_bookmarks and possibly other tags, the output is passed but the quotes become escaped instead of switched or stripped completely. I understand why this happens, I'm just not sure whether or not double quotes should actually be passed if user wishes to pass a class variable for their custom before or after arguments.

#11 @pishmishy
9 years ago

I've also noticed that wp_list_pages(),wp_list_bookmarks(), wp_list_categories() have a title_li argument, but wp_get_archives needs to be wrapped as:

<li><h2>Archives</h2>
    <ul>
        <?php wp_get_archives('type=monthly'); ?>
    </ul>
</li>

Can I also make an argument for the functions to be consistent in this fashion?

@pishmishy
9 years ago

Revised patch

#12 @pishmishy
9 years ago

Fresh patch, fixing the issues mentioned above, with no impact forseen on existing themes. Allows before/after on wp_list_pages() and wp_list_categories().

#13 @ryan
9 years ago

  • Owner changed from pishmishy to ryan
  • Status changed from assigned to new

#14 @Denis-de-Bernardy
8 years ago

there are several dups of this one

#15 @Denis-de-Bernardy
8 years ago

  • Component changed from General to Template

#16 @Denis-de-Bernardy
7 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 2.9 to Future Release

@wojtek.szkutnik
6 years ago

#17 @wojtek.szkutnik
6 years ago

  • Cc wojtek.szkutnik@… added
  • Keywords has-patch gsoc added

Apart from the after/before discussion, the archive title thing can be fixed.

#18 @bpetty
4 years ago

  • Keywords needs-patch gsoc removed

#19 @johnbillion
3 years ago

#3567 was marked as a duplicate.

#20 @nacin
3 years ago

  • Component changed from Template to Themes
  • Focuses template added

#21 @ryan
2 years ago

  • Owner ryan deleted
  • Status changed from new to assigned

#22 @wonderboymusic
14 months ago

  • Keywords needs-patch good-first-bug added; has-patch accessibility lists removed

#23 @igmoweb
11 months ago

Here's a refreshed patch.

There are some differences with the old one though.

  • wp_get_archives does not include a list_before and list_after arguments. IMO is easier to use a HTML wrapper instead of passing more arguments to the function. Easier to read too.
  • wp_list_categories has changed a lot during time and it would need a different approach.

@igmoweb
11 months ago

@igmoweb
11 months ago

Patch reviewed

@igmoweb
11 months ago

Unit tests

#24 @igmoweb
11 months ago

Added another patch.

Some fixes to the previous patch and also added back the list_before and list_after arguments in wp_get_archives as I had not seen clearly the point the first time.

#25 @atimmer
9 months ago

  • Keywords has-patch added; needs-patch removed

#26 @DrewAPicture
6 months ago

  • Owner set to igmoweb

Assigning to mark the good-first-bug as "claimed".

@igmoweb: Could you please combine 4969.3.diff and 4969.3.tests.diff into a single patch?

@igmoweb
6 months ago

Changes + Unit tests

#27 @igmoweb
6 months ago

Attached.

Thanks.

Note: See TracTickets for help on using tickets.