WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 5 months ago

Last modified 5 months ago

#25549 closed defect (bug) (fixed)

Twenty Fourteen: implement featured content "tag" mechanism

Reported by: lancewillett Owned by: lancewillett
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.8
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description (last modified by lancewillett)

Because the 3.8 "Featured Content" plugin needs more development help, and probably isn't going to be ready for 3.8, we're going with Plan B.

Goal: add a Featured Content "tag" option to Settings > Reading so authors can simply tag posts with a tag of their choosing to make them appear in the featured content area.

As discussed in October 8, 2013 office hours and after discussions with @photomatt on the direction of the theme and its main goal as a magazine theme.

Attachments (9)

25549.diff (16.4 KB) - added by obenland 6 months ago.
25549.2.diff (1.1 KB) - added by kovshenin 6 months ago.
25549.3.diff (1.1 KB) - added by kovshenin 6 months ago.
25549.4.diff (1003 bytes) - added by kovshenin 6 months ago.
25549.customizer.1.diff (11.8 KB) - added by celloexpressions 6 months ago.
Rough first pass at moving the featured content settings to the customizer. List of issues forthcoming.
25549.5.diff (16.0 KB) - added by obenland 5 months ago.
25549.6.diff (14.3 KB) - added by obenland 5 months ago.
25549.bug1.patch (1.5 KB) - added by rachelbaker 5 months ago.
25549.7.diff (9.0 KB) - added by obenland 5 months ago.

Download all attachments as: .zip

Change History (58)

comment:1 lancewillett6 months ago

  • Description modified (diff)

comment:2 iamtakashi6 months ago

  • Cc iamtakashi removed

comment:3 iamtakashi6 months ago

  • Cc takashi@… added

comment:4 kraftbj6 months ago

  • Cc bk@… added

comment:5 Frank Klein6 months ago

  • Cc contact@… added

Should the implementation in Twenty Fourteen offer the same options as Jetpack, i.e. choosing the maximum number of featured posts and the ability to hide the featured tag from displaying in post meta and tag clouds?

comment:6 MikeHansenMe6 months ago

  • Cc mdhansen@… added

obenland6 months ago

comment:7 obenland6 months ago

  • Keywords has-patch added; needs-patch removed
  • Version set to trunk

Patch takes advantage of the implementation in Jetpack, making existing data reusable with Twenty Fourteen.
Additionally, it allows custom solutions to override what's bundled with the theme.

comment:8 follow-up: lancewillett6 months ago

Patch looks good in testing. Only minor things I'd want to change in the include file, mostly in code comments. Probably setting default to 6 for Twenty Fourteen for "quantity" setting.

Tested with and without Jetpack activated, works great.

UX note: what do you think about changing the "Tag Name" entry to be a dropdown of tags from the site? Maybe the most used at the top. If not a dropdown, maybe an auto-complete so you don't have to remember or guess the tag.

comment:9 lancewillett6 months ago

In 25808:

Twenty Fourteen: first pass at implementing Featured Content settings, props obenland. See #25549.

kovshenin6 months ago

comment:10 kovshenin6 months ago

A dropdown of tags could easily get large and ugly. 25549.2.diff uses the existing .suggest script and ajax callback that's used in post.js.

comment:11 in reply to: ↑ 8 ; follow-up: obenland6 months ago

Replying to lancewillett:

Patch looks good in testing. Only minor things I'd want to change in the include file, mostly in code comments.

Right, a decision we still have to make is how far we want to deviate from what is in Jetpack. Currently it's only text domains and a strict notice fix (that will make its way upstream). Not bothering with the comments would allow us to update it a lot easier in the future, along the lines of Genericons.


Probably setting default to 6 for Twenty Fourteen for "quantity" setting.

This touches my comment above, too. I specified this in the add_theme_support() call to be compatible with existing data.


UX note: what do you think about changing the "Tag Name" entry to be a dropdown of tags from the site? Maybe the most used at the top. If not a dropdown, maybe an auto-complete so you don't have to remember or guess the tag.

kovshenin's patch seems fairly trivial and also something we could pass on upstream, I wouldn't mind doing that.

comment:12 in reply to: ↑ 11 lancewillett6 months ago

Replying to obenland:

Replying to lancewillett:

Right, a decision we still have to make is how far we want to deviate from what is in Jetpack.

Doh, I already made a bunch more changes to make it specific to Twenty Fourteen. Let's discuss more -- but my instinct is to not worry about keeping them in sync.

We can push things upstream without a big problem.

kovshenin's patch seems fairly trivial and also something we could pass on upstream, I wouldn't mind doing that.

Cool, let's test and get it in.

I think it's worth deviating from the Jetpack code and UX for the reason of improving the experience for our exact case -- and because the "FC as a plugin" approach isn't quite ready yet.

comment:13 follow-up: kovshenin6 months ago

Also, in inc/featured-content.php can we use uppercase __CLASS__ for consistency?

comment:14 in reply to: ↑ 13 ; follow-up: lancewillett6 months ago

Replying to kovshenin:

Also, in inc/featured-content.php can we use uppercase __CLASS__ for consistency?

Yep, patches welcome.

kovshenin6 months ago

comment:15 in reply to: ↑ 14 kovshenin6 months ago

Replying to lancewillett:

Yep, patches welcome.

25549.3.diff

comment:16 lancewillett6 months ago

In 25813:

Twenty Fourteen: implement tag auto-complete for featured content settings, and uppercase __CLASS__ for consistency. Props kovshenin, see #25549.

kovshenin6 months ago

comment:17 kovshenin6 months ago

25549.4.diff fixes a typo introduced in r25808.

comment:18 lancewillett6 months ago

In 25853:

Twenty Fourteen: fix a typo introduced in r25808, props kovshenin. See #25549.

comment:19 lancewillett6 months ago

Discussing in today's office hours (log) about where to put the UI controls for both the tag selection and the slider / normal layout options.

Everyone agreed it'd be better UX to have both options in the Customizer instead of one in Settings > Reading and the other in the Customizer.

comment:20 follow-up: celloexpressions6 months ago

I'm looking at the implementation for adding both controls to the customizer (with the slider one just saving the setting for now). Looking through the current implementation, a lot of it will need to be re-written, but the UI will be the same.

Since we're doing it there, does it make more sense to save the options as theme_mods, since they're all theme-specific? Only thing is that they need to be re-set if you switch to a child theme.

comment:21 in reply to: ↑ 20 ; follow-up: lancewillett6 months ago

Replying to celloexpressions:

Since we're doing it there, does it make more sense to save the options as theme_mods, since they're all theme-specific? Only thing is that they need to be re-set if you switch to a child theme.

I think theme mods are best, yes.

comment:22 follow-up: lancewillett6 months ago

Idea: what if the theme defaults to a tag of "featured" so that if a blog switches to Twenty Fourteen and has posts with that tag, they'll be automatically featured with no changes?

comment:23 in reply to: ↑ 22 sabreuse6 months ago

Replying to lancewillett:

Idea: what if the theme defaults to a tag of "featured" so that if a blog switches to Twenty Fourteen and has posts with that tag, they'll be automatically featured with no changes?

+1 to "featured" -- it's a reasonable default to pick, used by a number of other themes, and most importantly, having SOME default is going to make it a thousand times more likely that people will find the feature at all.

(disclaimer: made-up number. actual likelihood may vary. do not try this at home.)

comment:24 kraftbj6 months ago

Agreed. A default would be an easy win for our more notice users.

celloexpressions6 months ago

Rough first pass at moving the featured content settings to the customizer. List of issues forthcoming.

comment:25 follow-up: celloexpressions6 months ago

25549.customizer.1.diff is a rough first-pass at moving the settings to the customizer. Everything still works to the extent that it did previously, but I ran into several issues:

  • The layout and quantity options work as expected, but the tag name doesn't properly update in the customizer (it uses the tag id, which is updated when the tag name option is sanitized), and the hide-tag option doesn't update in the customizer (even when the tag is kept the same). The options do save just fine but you need to reload the customizer page to get them to update in the preview, even after they're live on the site when you save changes. Something's being cached where it shouldn't, I guess, although it didn't appear to be related to the setting of the transient in featured-content.php.
  • We should be running into same issue as generated accent colors (#25580) with the patched approach for setting the featured content tag id. Since we generate the tag id based on the tag name, similarly to the need to generate color variations, I'm starting to think that we should explore improved core support for generated options in the customizer... alternatively we could just do all of the extra filtering as in obenland's patch on #25580.
  • I'm not buying the add_theme_support approach. If the featured content file is designed to be a "module", including it should be enough to indicate a desire to support it in the theme. A solution for the quantity support (which I ignored for now) should be explored, though.
  • The new settings are added to customizer.php because they logically fit there and the customize_register code can't be put into a function in featured-content.php because that would result in nested class declarations (unless we put it outside of the class, in that file).
  • Speaking of that custom customizer control, perhaps there should be support for the number input type in core? While we're at it, range would be nice and could also utilize min and max.
  • When merging my implementation and lancewillett's of the layout settings, I had done it with a radio UI instead of select. That seems more logical to me because there are only two options anyway so it's better if you can see what they are right away (the select sets up an expectation for more than just one alternative, and feels like we're trying to hide the feature); anyone have thoughts on that? Patch also removes a couple of unneeded parameters there (they're the defaults).
  • ajaxurl not being defined in the customizer seems like a core bug. The suggest script doesn't appear to work even with the definition of ajaxurl, but it may just be super slow.
  • I didn't actually implement making a "featured" tag default, just threw it in as the default theme_mod value.

There are obviously a lot of details to work out, but I think this is a much better UI direction for this setting. If anyone has ideas for fixes (or discussions points) for any of the above (particularly the blatant bugs), patch away!

comment:26 in reply to: ↑ 21 ; follow-up: obenland6 months ago

Replying to lancewillett:

Replying to celloexpressions:

Since we're doing it there, does it make more sense to save the options as theme_mods, since they're all theme-specific? Only thing is that they need to be re-set if you switch to a child theme.

I think theme mods are best, yes.

If we go the theme mod route, we'd essentially make it non-portable between themes. Let's do it via option to make it pluggable and portable, what do you think?

comment:27 lancewillett6 months ago

Another quick idea I wanted to track, per feedback from Mel Choyce. If sticky posts exist, and no "featured" tag posts and no Featured Content tag has been set, use the sticky posts in the FC area.

This allows sites moving from other themes a quick and easy way to have Featured Content without doing anything -- it's a fallback to our smart defaults.

comment:28 lancewillett6 months ago

Following our discussion during today's office hours, @obenland is going to finish up this task and make sure we hit the following items:

  • Add "featured" tag as default tag. If user doesn't choose a tag, and has this one already, use it.
  • Add sticky posts as fallback to having no "featured" tag or any user-chosen tag at all.
  • Use an option—not theme_mod—to save the data for better portability and plugin integration.
  • Ensure all the UI needed to manage Featured Content exists in the Customizer.

comment:29 in reply to: ↑ 26 ; follow-up: dd326 months ago

Replying to obenland:

If we go the theme mod route, we'd essentially make it non-portable between themes. Let's do it via option to make it pluggable and portable, what do you think?

If we want something that's portable between themes, we should be looking into adding a core API rather than just using a option, IMHO a theme shouldn't just use an option because it wants to share it with other themes.

comment:30 in reply to: ↑ 29 obenland6 months ago

Replying to dd32:

If we want something that's portable between themes, we should be looking into adding a core API rather than just using a option, IMHO a theme shouldn't just use an option because it wants to share it with other themes.

Agreed, which is why we're working on a Featured Content plugin (see ticket description above). If we can make 2014's implementation compatible with existing featured content solutions by just using an option rather than theme mods, I don't see a reason not do that though.

comment:31 in reply to: ↑ 25 obenland5 months ago

Replying to celloexpressions:

The suggest script doesn't appear to work even with the definition of ajaxurl, but it may just be super slow.

It's actually the admin CSS. The z-index is set to 10000, it needs to be a lot higher than this to show though.

obenland5 months ago

comment:32 obenland5 months ago

Latest patch moves FC settings into the customizer. We should decide if we prefer not to use the suggest script or add an inline style to make it work in the customizer.

comment:33 celloexpressions5 months ago

Created #25866 and #25867 to address the core bugs that are problematic for our implementation here (for the suggest script).

See additional discussion in the IRC office hours. We decided to switch the layout-changing UI back to a select dropdown.

obenland5 months ago

comment:34 obenland5 months ago

Updated patch.

comment:35 lancewillett5 months ago

In 26037:

Twenty Fourteen: move all controls for managing Featured Content to the Customizer. Also implement smart fallbacks to not using a custom tag with the "featured" tag and sticky posts. Props obenland and celloexpressions, see #25549.

comment:36 lancewillett5 months ago

Things to finish this one up:

  • Debug why in some cases the display doesn't refresh, in Customizer and in front-end, after changing the tag option value
  • Get the inline CSS working to have the auto-suggest tag list display properly.
Last edited 5 months ago by lancewillett (previous) (diff)

comment:37 lancewillett5 months ago

See #25867 for the CSS for the auto-suggest tag (has patches from celloexpressions for both MP6 and old admin CSS.

comment:38 celloexpressions5 months ago

Patch added to #25866 for moving the ajaxurl definition in the customizer into core.

comment:39 follow-up: lancewillett5 months ago

Steps to repeat a few bugs with the Featured Content option in the Customizer.

First bug

Posts: set up your test install so that no posts are tagged "featured" and no sticky posts.

  1. Open Customizer, open Featured Content pane.
  2. Tag name "featured" is in the input.
  3. Change the tag to something you do have tagged, with Unit Test sample data "Post Formats" works.
  4. Save in Customizer (notice here it doesn't refresh the Customizer iframe version of the site -- that's bug 2 below)
  5. Go to front end, refresh. The posts show up as expected in Featured Content area.
  6. Open Customizer again, in Featured Content pane clear out the tag and save.
  7. Go to front end, refresh. Post Formats posts are still in Featured Content area! Should be empty instead.

The only way to clear the front-end view is to make any change in the Customizer and save again. Otherwise the previously selected tag sticks even after clearing the input.

I think it's related to saving the Featured Content input with an empty value.

Second bug

When you change Featured Content tag in the Customizer, the preview iframe doesn't refresh with the correct posts.

Last edited 5 months ago by lancewillett (previous) (diff)

rachelbaker5 months ago

comment:40 in reply to: ↑ 39 rachelbaker5 months ago

Replying to lancewillett:

Steps to repeat a few bugs with the Featured Content option in the Customizer.

First bug

Posts: set up your test install so that no posts are tagged "featured" and no sticky posts.

  1. Open Customizer, open Featured Content pane.
  2. Tag name "featured" is in the input.
  3. Change the tag to something you do have tagged, with Unit Test sample data "Post Formats" works.
  4. Save in Customizer (notice here it doesn't refresh the Customizer iframe version of the site -- that's bug 2 below)
  5. Go to front end, refresh. The posts show up as expected in Featured Content area.
  6. Open Customizer again, in Featured Content pane clear out the tag and save.
  7. Go to front end, refresh. Post Formats posts are still in Featured Content area! Should be empty instead.

Found that when the text field for the Featured Content setting tag_name was emptied, the logic in the settings callback method validate_settings() was not clearing the tag_id. Updated the settings processing logic for the tag_name and tag_id settings in 25549.bug1.patch

obenland5 months ago

comment:41 obenland5 months ago

Updated patch simplifies customizer implementation and validation.

comment:42 lancewillett5 months ago

Thanks rachelbaker and obenland, looking better.

@obenland, I noticed $wp_customize->add_section for Featured Content section is in both customizer.php and featured-content.php now. Can we remove one of the versions?

comment:43 lancewillett5 months ago

I can confirm both bugs above are fixed. w00t. This interaction is working really nicely.

comment:44 lancewillett5 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 26079:

Twenty Fourteen: improve Featured Content experience in the Customizer. Better preview and cache clearing, props obenland and rachelbaker. Fixes #25549.

comment:45 follow-up: celloexpressions5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It's definitely working a lot better!

There's still some undesired behavior, particularly when you type very slowly, or wait for the suggest script to load. If you start typing a word, then wait for the suggestions, the customizer will sometimes update to the thing that was typed before you click on a suggestion (often resulting in it falling back to sticky posts), then not update once you click on a the suggestion. This seems particularly problematic with multi-word tags; "Post Formats" frequently ends up registering "post" instead.

This would probably be acceptable (though not ideal) behavior, but empty tags are created for each term that runs through the validation function. We need to find a way to prevent partially spelled tags from creating empty tags. If we can make the suggest script run faster, that might help.

comment:46 in reply to: ↑ 45 obenland5 months ago

We could decrease the timeout before the ajax request is made. Or remove the suggest script.

comment:47 follow-ups: lancewillett5 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

I'd prefer if you didn't re-open tickets. :)

Feel free to open a new ticket to address improving the Ajax interaction of the tag suggestion.

comment:48 in reply to: ↑ 47 celloexpressions5 months ago

Replying to lancewillett:

I'd prefer if you didn't re-open tickets. :)

Feel free to open a new ticket to address improving the Ajax interaction of the tag suggestion.

Sorry, I was actually writing the comment as you committed & closed, so it hit a concurrent edit conflict :)

The UI is fine, the only issue is really that we might get bad results with creating the tag if it doesn't already exist. Not sure that there's a way we could do anything about that, though, so I think we can just leave it as-is for now and see if anyone has issues with it.

comment:49 in reply to: ↑ 47 SergeyBiryukov5 months ago

Replying to lancewillett:

Feel free to open a new ticket to address improving the Ajax interaction of the tag suggestion.

Follow-up: #26080

Note: See TracTickets for help on using tickets.