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

Simplify sorting of table data #362

Merged
merged 2 commits into from
Apr 27, 2015
Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Apr 27, 2015

No description provided.

@jrfnl jrfnl added this to the 2.5.0 milestone Apr 27, 2015
@GaryJones
Copy link
Member

Could this sorting functionality be pulled out into a new function, so it could potentially be unit tested?

Current function to gather the data, another to sort it in a specific way. Might then allow others to filter the callback so that they could sort it in another way.

Also adds `tgmpa_plugin_table_items` hook on which the sorting is done.
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 27, 2015

Good point: done!

$name = array();

foreach ( $items as $i => $plugin ) {
$type[ $i ] = $plugin['type'];
Copy link
Member

Choose a reason for hiding this comment

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

Where is this 'type' value originally being added from? I don't see it in example.php, and we don't populate it in register().

@GaryJones
Copy link
Member

Certainly better than what we have, but something is niggling me about it.

It orders by "Required" then "Recommended" (descending type), but in other languages these words may be in the reverse order.

Also, it doesn't account for any third option that we may want to add later.

Can there be something like:

// Main class
/**
 * Possible types.
 *
 * Ordered by the order they should appear in the plugin table (i.e. Required top by default).
 */
public function get_types() {
    return apply_filters(
        'tgmpa_types',
        array(
            'required'    => __( 'Required', 'tgmpa' ),
            'recommended' => __( 'Recommended', 'tgmpa' ),
        )
    );
}

And then grab and use this array to check the key and multisort accordingly?

That allows authors to change the order via filter, change all the wording (if we also pull from this method when creating table rows) without filtering gettext or translating, and allows us an easy way to add new types in the future.

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 27, 2015

While I agree with your line of thinking and had been wondering about the i18n aspect as well, I think that is a change which should be in another pull request.
This one really is about simplifying the sorting code (of which most was redundant, never used as it was wrongly set up).

Your point is about abstracting advise types and we need to put some more thought into that as I believe we should also abstract the logic for determining the type (sort of duplicated in some places) and if we abstract it, think about things like a 'Buy' type where there would be no install link.
And to be fair, if we take that into account, it sounds more like a 3.0 feature to me (the type abstraction, not the sorting simplification).

GaryJones added a commit that referenced this pull request Apr 27, 2015
@GaryJones GaryJones merged commit 43b9384 into develop Apr 27, 2015
@GaryJones GaryJones deleted the feature/simplify-table-sorting branch April 27, 2015 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants