Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#42421 closed enhancement (fixed)

Move "Filter by" strings into labels arrays of registrations

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Administration Keywords: good-first-bug has-patch commit has-dev-note
Focuses: administration Cc:

Description

WordPress admin includes a few "Filter by" strings. They are largely used in screen-reader text.

  • "Filter by category" is used 2 times
  • "Filter by type" is used 1 time in media
  • "Filter by comment type" is used 1 time in comments
  • "Filter by date" is used once as a generic List Table string

The first 3 could be included as part of the post type and taxonomy labels. The last 1 is in the months_dropdown() method in the base list table class, and I guess could go either way.

The benefit of adding these to the labels arrays is these strings can be re-used in other places, not repeated, and custom post types and taxonomies won't need to hard-code their strings in with their mark-up, and can instead register them with the other labels.

Attachments (3)

42421.patch (3.9 KB) - added by nicolalaserra 6 years ago.
In this patch I add two new labels ( filter_by_category and filter_by_date ) inside get_post_type_labels in orther to avoid using it directly in views and thus having them duplicated.
42421.1.diff (4.4 KB) - added by audrasjb 3 years ago.
Patch refresh and small PHP fix
42421.2.diff (5.6 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (20)

#1 @johnbillion
6 years ago

  • Keywords good-first-bug added

#2 @SergeyBiryukov
6 years ago

  • Component changed from General to Administration

@nicolalaserra
6 years ago

In this patch I add two new labels ( filter_by_category and filter_by_date ) inside get_post_type_labels in orther to avoid using it directly in views and thus having them duplicated.

#3 @Junaidkbr
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#4 @DrewAPicture
6 years ago

  • Keywords dev-feedback added
  • Owner set to nicolalaserra
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed". @johnjamesjacoby Care to follow up here with some feedback? :-)

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#6 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.7
  • Owner changed from nicolalaserra to SergeyBiryukov
  • Status changed from assigned to reviewing

@audrasjb
3 years ago

Patch refresh and small PHP fix

#7 @audrasjb
3 years ago

  • Keywords needs-testing dev-feedback removed

Hi,
I added a small refresh above. Indeed, the patch was not applying cleanly against Trunk.
Plus, I added a small PHP fix on src/wp-admin/includes/class-wp-list-table.php:

<label for="filter-by-date" class="screen-reader-text"><?php get_post_type_object( $post_type )->labels->filter_by_date; ?></label>

was not displaying anything. I added the necessary echo.

I tested the patch the patch and I didn't noticed any change on the "Filter by" screen reader text, so I guess we're good to go.

Cheers,
Jb

#8 @audrasjb
3 years ago

  • Keywords needs-dev-note added

Adding this to the miscellaneous changes dev note list.

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


3 years ago

#10 @audrasjb
3 years ago

@SergeyBiryukov could you please add that one to your review-list?
Feel free to punt it if you don't think it's realistic for 5.7 beta 1 :)

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#12 @hellofromTonya
3 years ago

  • Keywords commit added

Per Sergey:

Looks good at a glance, I think this can be committed before Beta 1.

Marking for commit.

Last edited 3 years ago by hellofromTonya (previous) (diff)

#13 @SergeyBiryukov
3 years ago

Thanks for the patch!

Since the new filter_by_category label for taxonomies would only be used for the Links Manager feature deprecated in #21307, I think we should probably skip it for now and just go with the post type labels.

#14 follow-up: @johnjamesjacoby
3 years ago

This is coming along nicely!

I think the “Filter by Category” string needs to be part of the registered Taxonomy instead, and not the Post Type. The current patch would require every registered Post Type to have pre-existing knowledge of the Taxonomies that are registered to it, which is unlikely to always be the case.

Thoughts?

#15 in reply to: ↑ 14 @SergeyBiryukov
3 years ago

Replying to johnjamesjacoby:

I think the “Filter by Category” string needs to be part of the registered Taxonomy instead, and not the Post Type.

Good point :) I think 42421.2.diff would be the way to go then.

#16 @SergeyBiryukov
3 years ago

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

In 50120:

Posts, Post Types: Introduce "Filter by date" and "Filter by category" as post type and taxonomy labels, respectively.

This provides a more consistent location for these strings and allows for reusing them in other places without hardcoding them in the markup.

Props nicolalaserra, audrasjb, johnjamesjacoby, SergeyBiryukov.
Fixes #42421.

Note: See TracTickets for help on using tickets.