Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use $column_name instead of $column_title and deprecate redundant filter #1519

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

delawski
Copy link
Contributor

Fixes #1295.

The issue originally described in #1184 has been addressed by #1185. According to #1295, the issue was still present.


Adding custom columns to the Stream list table consisted of 3 steps:

  1. Hooking into the wp_stream_list_table_columns filter, where a new column name (slug) and title (label) should be added.
  2. Hooking into the wp_stream_register_column_defaults filter, where the column name (slug) should be added again.
  3. Hooking into the wp_stream_insert_column_default_{$column_name} filter, where the column default value should be returned.

The issue described in #1295 and the solution proposed in https://github.com/xwp/stream/pull/1185/files#r731374767 replaced the the $column_title with the $column_name in the wp_stream_insert_column_default_{$column_name} filter. This is the correct and desired behavior since we want to use a name (slug) instead of a title (label) in the hook name.

When reviewing the entire process of adding new columns, I realized that the second step (as described above) is redundant and can be dropped. There is no need to register column names in the wp_stream_register_column_defaults again. This is already done in wp_stream_list_table_columns (please correct me if I'm wrong).

Because of that, I decided to deprecate wp_stream_register_column_defaults filter so that the current process for adding new custom column looks like this:

  1. Hook into the wp_stream_list_table_columns filter, where a new column name (slug) and title (label) is added.
  2. Hook into the wp_stream_insert_column_default_{$column_name} filter, where the column default value is returned.

Checklist

  • Project documentation has been updated to reflect the changes in this pull request, if applicable.
  • I have tested the changes in the local development environment (see contributing.md).
  • I have added phpunit tests.

Release Changelog

  • Fix: Use $column_name instead of $column_title in the wp_stream_insert_column_default_{$column_name} filter
  • Deprecate: wp_stream_register_column_defaults filter in favor of wp_stream_list_table_columns

@delawski delawski added the bug label Jul 24, 2024
@delawski delawski added this to the 4.0.1 milestone Jul 24, 2024
@delawski delawski requested a review from tharsheblows July 24, 2024 15:25
@delawski delawski self-assigned this Jul 24, 2024
*
* @return string
*/
$out = (string) apply_filters( "wp_stream_insert_column_default_{$column_name}", $out, $record, $column_name );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 note: This is where the main fix is: note the change from $column_title to $column_name in the hook name.

/* translators: %s is the Stream version number. It is part of a filter deprecation notice and is preceded by: "{hook_name} is deprecated since version %s of Stream". */
sprintf( __( '%s of Stream', 'stream' ), '4.0.1' ),
'wp_stream_list_table_columns',
__( 'Usage of this filter is redundant. Instead, define custom column name and title using the `wp_stream_list_table_columns` filter and provide the default value using the `wp_stream_insert_column_default_{$column_name}` filter.', 'stream' )
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 chore: Please help me refine the copy here. I'm not sure if that sounds well (and correct in general).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ...

This filter is being deprecated as it is redundant. You can define custom column names and titles using the `wp_stream_list_table_columns` filter then provide the value for the custom columns using the `wp_stream_insert_column_default_{$column_name}` filter. 

Copy link
Contributor

@tharsheblows tharsheblows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a suggestion on the deprecation text as requested.

When I tested against the issue which started it all , the table layout was off but I think that can be dealt with in the future.

@delawski
Copy link
Contributor Author

When I tested against the issue which started it all , the table layout was off but I think that can be dealt with in the future.

Thank you for the catching that layout issue. The math behind the column widths did not account for having custom columns. While adding just 1 custom column would make the summary column quite narrow, having 2 or more custom columns would reduce the summary to an unreadable strip. I also noticed that the date column did not behave well if the screen was narrower (or when custom columns were present). I fixed both problems in 31bcb0f.

Here are some screenshots of how the table is laid out on different screens when there are 2 custom columns present:

List Table
Screenshot 2024-07-25 at 10 11 44
Screenshot 2024-07-25 at 10 11 52
Screenshot 2024-07-25 at 10 12 01

It'll be the responsibility of the plugin extender to further adjust this default layout.

For backwards compatibility, the new selector has the same specificity as the previous one (0,2,0). This way if someone already adjusted the column widths in their implementation, their customizations should stay intact:

Screenshot 2024-07-25 at 10 21 48

@delawski delawski merged commit 77f8340 into develop Jul 25, 2024
2 checks passed
@delawski delawski deleted the fix/1295-list-table-columns branch July 25, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding multiple columns to the stream table using filters HTML never loaded RE: #1184
2 participants