feat: add support for displaying tags as labels#4381
feat: add support for displaying tags as labels#4381jason10lee wants to merge 19 commits intotrunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to display selected post tags as labels, similar to how categories are displayed. The feature includes an admin interface for enabling tags as labels with optional custom label text, and works in conjunction with a companion PR in the newspack-theme repository.
- Added a new Tag_Labels class that manages tag label settings via term meta
- Created admin UI fields to enable tags as labels and customize label text
- Added helper methods for themes to retrieve tag labels for display
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| src/other-scripts/tag-labels/index.js | Adds jQuery-based client-side logic to enable/disable the label flag input based on checkbox state |
| includes/tag-labels/class-tag-labels.php | Implements the core Tag_Labels class with admin UI, data storage, and helper methods for retrieving tag labels |
| includes/class-newspack.php | Includes the new tag-labels class file to integrate it into the plugin |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
leogermani
left a comment
There was a problem hiding this comment.
This is looking good Jason, thanks for the work! I left some minor comments in the code.
I'll tag @thomasguillot and add a needs-design review for him to have a look and answer your questions around presentation. I think it should follow what Sponsors do as much as possible (which is to display the label above the title). Thomas, please also share your thought on how we name and present this feature in the UI. Is "Labels" a good name?
I also have an existencial question I'll copy for @laurelfulford : Since this is very tied to the theme, should this whole thing live in the theme instead? WDYT?
How come we don't have the checkbox and the label flag field when first creating? I must admit I thought I hadn't switched branch and it wasn't working at first. |
Thanks, @thomasguillot! That was a potential issue I flagged, and based on your and @leogermani's comments, I think it'll be worth making those options visible on creation. |
|
"Labels" works well. It's intuitive, distinct from the existing "Tags" terminology, and commonly understood in editorial contexts I think. For the fields, I'd suggest: (ae531b4) Checkbox: "Display as label" with help text: Show this tag as a highlighted label wherever posts are displayed. |
All good, and I'll be using the tweaks you made to the dependent field toggles for the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hi @jason10lee finally got back to these PRs to give it another go. It's looking good. On the content loop, when I hover the label, the background color doesn't change. In the single, it changes. I don't know which is the correct behavior. I had the impression that there is too much code duplication on the newspack blocks plugin. Especially when it comes to build the HTML, which is basically the same HTML used in other places. It would be awesome if this would live in one single place - ideally the plugin itself. (Idea for the future - not this PR - we could have an api/hooks in the block and the themes to add labels on the top of posts title. and then both sponsors and tag labels could do things the same way... but just an idea for now) |
Thanks, @leogermani! Good question as to which is right. I'll see what sponsors does here and standardize on that (if there is a standard!).
Yeah, I started out with an idea like that--the HTML generation code would live in one place, and I'd just have thin accessor functions elsewhere to just reuse it. The HTML needed for single-post display wound up being different from that required for Content Loop, so I scrapped the idea. Still, the idea of a factory function for things like labels and badges is really appealing. I think that'll become super doable once Block Theme is out in the wild, so let's revisit that soon! |
Maybe I'm not seeing it, but the only difference I see is that one is wrapped around a div and another one around a span. If that's the only difference, I think we can still have one single function with a param to inform the desired wrapper element. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Hey, @adekbadek! Thanks for the reviews on this and its siblings! Leo made a similar note about reducing the maintenance surface (and you've probably guessed I had similar plans from the parameterization in the canonical HTML generation) so I think we've got a solid direction here. Will be landing patches on these three topic branches later today to address that, and will dig into the other feedback. |
Yep! That was me having second thoughts about splitting this up like we do with sponsors (that's also the reason the class attributes are parameterized). Updated this and related projects to centralize in b8b0e0f. |
All Submissions:
Changes proposed in this Pull Request:
This PR is one of a set that adds the ability to display selected tags as labels. Changes live in the
feat/tag-labelstopic branch on the newspack-blocks, newspack-plugin, and newspack-theme projects.See also: Automattic/newspack-theme#2613
See also: Automattic/newspack-blocks#2282
In non-singular context (as part of a Content Loop block, for example), the tag label will show up in front of the headline.In singular context (on the individual post page, for example), the tag label will show up with the category label.By default, the text of the tag label will be the same as the tag name. Optionally, the label text can be customized (e.g., to something shorter and more suitable for use as a label).Labels will appear above the headline, after any category or sponsorship labels.
By default, the text of the tag label will be the same as the tag name. Optionally, the label text can be customized (e.g., to something shorter and more suitable for use as a label). Additionally, the label color can be customized in the Newspack Theme customizer.
Addresses NPPD-383: "Breaking News" or "Developing" Label.
How to test the changes in this Pull Request:
Basic operation:
Multiple labels:
Custom label text (flag):
Custom label color:
Other information: