WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26821 closed defect (bug) (fixed)

Hook Docs (14): wp-admin/includes/screen.php

Reported by: enej Owned by: DrewAPicture
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch commit
Focuses: docs Cc:

Description

Attached patch has doc for filters and action found in the file wp-admin/includes/screen.php

Attachments (4)

26821.diff (6.0 KB) - added by enej 4 years ago.
Hook docs for file wp-admin/includes/screen.php
26821.2.diff (7.8 KB) - added by enej 4 years ago.
Second pass at screen.php file
26821.3.diff (6.4 KB) - added by kpdesign 4 years ago.
Third pass
26821.4.diff (6.5 KB) - added by DrewAPicture 4 years ago.
Final pass

Download all attachments as: .zip

Change History (15)

@enej
4 years ago

Hook docs for file wp-admin/includes/screen.php

#1 @kpdesign
4 years ago

  • Keywords has-patch added
  • Summary changed from Hook Docs: wp-admin/includes/screen.php to Hook Docs (14): wp-admin/includes/screen.php

#2 @jeremyfelt
4 years ago

  • Component changed from Inline Docs to Bootstrap/Load
  • Focuses docs added

#3 @kpdesign
4 years ago

  • Owner set to kpdesign
  • Status changed from new to assigned

#4 @DrewAPicture
4 years ago

  • Keywords needs-patch added; has-patch removed

Hi, thanks for the patch. Here some feedback for the next round of changes:

Overall:

  • All descriptions should start with a capital letter, and end with a period. The same goes for parameter descriptions.
  • All @since versions should follow the 3-digit x.x.x style
  • Remove the functional docs changes on the show_screen_option() method. If you'd like to patch these changes and submit them in a different ticket, we can review those improvements separately. For now, those changes should be removed from the patch here :)

manage_$screen->id_columns filter:

  • Change the description to "Filter the column headers for a list table on a specific screen."
  • As this is a "dynamic" hook, we need to outline what the dynamic portion, e.g. $screen->id represents in a long description.
  • We should be using a docs-specific variable for the single parameter here. I suggest something like $columns

default_hidden_meta_boxes filter:

  • As part of the suggested language for filters and actions, you should use 'Filter X', rather than 'Filters X'. The filter itself doesn't filter anything :)
  • Parameter descriptions should start with a capital letter and end with a period.

hidden_meta_boxes filter:

  • 'Filter' instead of 'Filters' in the description
  • Parameter descriptions should start with a capital letter and end with a period.

current_screen hook:

  • "Fires after the current screen has been set."
  • Needs a period at the end of the parameter description.

contextual_help_list, contextual_help, and default_contextual_help filters:

  • Should probably use an @deprecated tag here for 3.3.0 and point out which hook or method should be used instead.
  • Parameter descriptions, variables, and formatting are missing.

screen_layout_columns filter:

  • The "// Back compat for plugins using the filter instead of add_screen_option() comment should be moved and incorporated into the long description for this filter's docs.
  • 'Filter' instead of 'Filters' in the description.
  • Parameter descriptions, variables, and formatting are missing.

screen_settings filter:

  • 'Filter' instead of 'Filters'.
  • What are "screen settings"? Can you be more descriptive here?
  • Parameter descriptions, variables, and formatting are missing.

screen_options_show_screen filter:

  • Replace 'weather' with 'whether'.
  • Parameter descriptions, variables, and formatting are missing.

comments_per_page, edit_categories_per_page, edit_posts_per_page filters:

  • You need to be specific about which per page setting each filter is addressing. Are these in admin screen only, only for specific object types?
  • Parameter descriptions should start with a capital letter and end with a period.

$option filter:

  • You can mark this as a duplicate with:
    /** This filter is documented in wp-admin/includes/class-wp-list-table.php */
    

That should be good for round one. Looking forward to round two :)

#5 @enej
4 years ago

Thank you so much Drew, for your feedback.
I will try to update this patch later this week.

@enej
4 years ago

Second pass at screen.php file

#6 @kpdesign
4 years ago

  • Keywords has-patch added; needs-patch removed

@kpdesign
4 years ago

Third pass

#7 @kpdesign
4 years ago

  • Owner changed from kpdesign to DrewAPicture

26821.3.diff contains language and formatting changes, and fixes for duplicate hooks.

This needs a secondary review and a recommendation.

#8 @enej
4 years ago

26821.3.diff reads way better. Thank you Kim!

The only thing that I found is manage_$screen->id_columns filter was around since 2.7.0 see http://svn.automattic.com/wordpress/tags/2.7/wp-admin/includes/template.php line 841.

Instead of $screen->id WordPress used $page.

@DrewAPicture
4 years ago

Final pass

#9 @DrewAPicture
4 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.9

#10 @DrewAPicture
4 years ago

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

In 27256:

Inline documentation for hooks in wp-admin/includes/screen.php.

Props enej, kpdesign.
Fixes #26821.

#11 @SergeyBiryukov
4 years ago

  • Component changed from Bootstrap/Load to Administration
Note: See TracTickets for help on using tickets.