-
Notifications
You must be signed in to change notification settings - Fork 2
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
[MOOSE-52] Terms Block #81
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of quick comments but everything looks pretty good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an excellent take on the block. A few changes & suggestions, if you'd be so kind.
Closing in favor of moving the Terms block to Moose Resources: moderntribe/moose-resources#3 Open discussion items here have been moved to TODO list in the new PR. New Moose PR has been opened to fix a few issues that cropped up: #105 |
Reopening to continue iterating within Moose. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeoffDusome, I will put together a quick PR to look into the PHP in the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be good from the PHP side now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed up on a couple existing conversations. We're really close here, Geoff. Thanks for tackling this!
@dpellenwood should be good for another look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:chefskiss: This is excellent. merge away!
What does this do/fix?
taxonomyToUse
attribute is currently pulling all available taxonomies. I think I could maybe edit this to pull only taxonomies that are attached to the current post type?onlyPrimaryTerm
attribute determines if we should grab the full list of terms or only the "primary" term. If Yoast is enabled on the site and we're using the "Category" taxonomy, the code will look for the primary category as designated by Yoast, with a fallback to the first category in the list. If Yoast is not enabled, we just grab the first term in the list returned.hasLinks
attribute determines if the terms should display as links or plain texttermStyle
attribute determines if the terms should display as basic text or as pill shaped "tags"For reference, the code that pulls the taxonomies looks like this:
There's not a whole lot of documentation on
useSelect
, so I'm just kinda going by what I'm finding online. I would assumegetTaxonomies
has more attributes thanper_page
but I'll have to look for them.This PR closes #66
QA
Link to issue: https://moderntribe.atlassian.net/browse/MOOSE-52
Screenshots/video: