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

Revisit the Taxonomy List widget #1171

Closed
wants to merge 14 commits into from
Closed

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Mar 17, 2016

Changes

  • Remove option to include only chosen terms
  • Include ability to exclude chosen terms from returned terms
  • Remove ability to output infinite terms, number output must be set and defaults to 5
  • Make the taxonomies option a select instead of a text field, preventing empty-taxonomy error
  • Add option to choose ascending or descending sort order, and docs: default is descending
  • Add option to choose sort-by argument on get_terms query: default is 'id'
  • Creates two sort options:
    • alphabetically by term name
    • newest term first, by descending order of term id
  • Improve docs in the widget's settings

screen shot 2016-03-16 at 10 24 32 pm

The combined defaults of descending by ID mean that the default is to show the most-recently-created terms in the taxonomy.

Why

To make the

For HELPDESK-589

What else?

This does not include the option to sort by last-updated; WordPress does not provide this option. 😞

…de ability to exclude chosen terms, make the taxonomies option a select instead of a text field, improve docs inside and outside the plugin
@aschweigert
Copy link

We don't need the option for ascending/descending, reverse alphabetical order would never really make sense here. We also really do need the option to order by most recently updated (or maybe created?) term. And that's the only other option we need. So: alphabetical or most recent. That's it. Keep this simple.

@aschweigert aschweigert assigned benlk and unassigned aschweigert Mar 17, 2016
@benlk
Copy link
Collaborator Author

benlk commented Mar 17, 2016

Simplified it to two options:

  • Sort by alphabetical order (name in ascending)
  • Sort by most-recently-created (id in descending)

screen shot 2016-03-16 at 11 56 42 pm

I think there's an argument for an oldest-term-first option, because sometimes the oldest terms will be the biggest buckets: things like a "News" or "Opinion" or "Very Important To Our Brand" term.

@aschweigert
Copy link

A couple things:

  1. remove all these inline docs, put them in the regular widget docs and (possibly?) link to them
  2. the various render term functions can be combined into one function, you can use the tax_query method of getting posts for all of these so I'm not sure why you need four separate, mostly duplicative functions

@benlk
Copy link
Collaborator Author

benlk commented Mar 17, 2016

To avoid merge conflicts, I'm going to add the docs to Jack's docs branch: https://github.com/INN/Largo/pull/1160/files#diff-09863b072aa8e2e4e3eb31c0735a1b92R178

The series rendering function has a lot of logic that isn't needed by the other terms, so that's why it should remain separate. But yes, the rest can be combined.

@benlk
Copy link
Collaborator Author

benlk commented Mar 17, 2016

Inline docs simplified, category and term rendering removed, and a new issue found: #1173

…(), improve its registration args when it's disabled. Remove unregister_post_types_taxonomy() and Largo_Term_Icons::register_taxonomy()
@benlk
Copy link
Collaborator Author

benlk commented Mar 17, 2016

Latest commits include a fix for #1173 and a more=thorough description of what needs to be done in the test for the function.

@benlk benlk assigned aschweigert and unassigned benlk Mar 17, 2016
@aschweigert aschweigert removed their assignment Mar 18, 2016
@@ -443,29 +474,3 @@ function largo_first_headline_in_post_array($array) {

return $headline;
}

Choose a reason for hiding this comment

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

why is this section removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unregister_post_types_taxonomy's check was moved into general taxonomy-registration function in inc/taxonomies.php. It was moved there for consistency with other taxonomy-registration functions, but in any case it needed to be integrated with the function that registered the post-types taxonomy, instead of registering a taxonomy without any labels.

I'm not sure why unregister_series_taxonomy was removed; but the same registration fix should be applied to the series taxonomy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR has been updated: The unregister_series_taxonomy code has been placed into largo_custom_taxonomies where the series taxonomy is registered.

@benlk
Copy link
Collaborator Author

benlk commented Mar 23, 2016

  • Implement unregister_series_taxonomy in the same style as the change to the post-types taxonomy

or

  • put unregister_series_taxonomy back

@aschweigert
Copy link

probably not urgent enough to do this as a hotfix, can you resubmit against develop and we'll include it in the next release?

@benlk
Copy link
Collaborator Author

benlk commented Mar 31, 2016

Refiled as #1187.

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.

None yet

3 participants