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

feat: feature extensions from gdg #120

Merged
merged 7 commits into from
Sep 20, 2021
Merged

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Sep 14, 2021

All Submissions:

Changes proposed in this Pull Request:

Ports some of the lower-risk feature enhancements developed for GDG back into the main Listings plugin. These new features are gated behind new settings so that sites that are already using Listings won't see any unexpected behavior.

I'd also like to port the "featured listing" functionality from GDG, but I'm awaiting some feedback on https://github.com/Automattic/newspack-gdg/issues/1 before we do so as it may impact performance in the current state.

How to test the changes in this Pull Request:

Yoast primary categories

  1. Check out this branch; go to Listings > Settings.
  2. Observe a new setting under the "Post Meta Settings" heading, disabled by default:

Screen Shot 2021-09-14 at 12 11 29 PM

  1. Enable it and save. Edit any listing and expand the "Categories" sidebar, and confirm that you can no longer choose a primary category.

Listing archives

  1. Check out this branch; ensure that category/tag archives don't contain listing posts and list post type archives (with slugs as defined in Listings > Settings) 404, as these features haven't been explicitly enabled yet.
  2. Go to Listings > Settings and observe some new settings under a "Directory Settings" heading (both should be disabled by default):

Screen Shot 2021-09-14 at 11 57 09 AM

  1. Enable the first option, "Enable listing archives." Note that permalinks should be flushed automatically if you change this setting.
  2. Make sure you have some listings published and assigned a.) a category/tag that is shared by other non-listing posts, and b.) a category/tag that is assigned only to listings
  3. Visit the term archives for the categories/tags assigned to your listings. Confirm that listing posts now appear in these archives, alongside other post types if the term is shared. The styles at this point should be identical to regular WP archives.
  4. Visit the post type archives for a listing (e.g. default slug for places: /listings/places/). Confirm that these now work as well.
  5. Go back to Listings > Settings and enable the second new option, "Show listing archives as grid".
  6. Refresh the category/tag archive for the term that is only applied to listings (no other post types). Confirm that the items are laid out in a grid at larger viewports. This option only applies to archive pages when every item is a listing.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo dkoo added the enhancement New feature or request label Sep 14, 2021
@dkoo dkoo requested a review from a team September 14, 2021 18:10
@dkoo dkoo self-assigned this Sep 14, 2021
@dkoo dkoo changed the title Feat/feature extensions from gdg feat: feature extensions from gdg Sep 14, 2021
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Working as described!

A few questions/suggestions:

  • With listing archives disabled, a category assigned only to listing posts does not display content. Is this expected behavior? I'd expect that this type of category would display content regardless of my listing archive options.
  • Why are listing archives disabled by default?
  • I suggest the archive options to be higher up on the page, right below the permalink inputs

@dkoo
Copy link
Contributor Author

dkoo commented Sep 20, 2021

@miguelpeixe Great questions! Maybe we can talk through the best possible solution here. 😄

Why are listing archives disabled by default?

I'll start with this question. Back when we first released v1, we decided to make listing CPTs use regular post categories and tags instead of custom taxonomies. This was intended to make it easier to cross-reference listings and posts as related content. For the sites that are using Listings in production (admittedly few at the moment), if any are assigning the same categories and tags to listings and posts, enabling listing archives by default would cause post archives for those sites to start displaying listings intemixed with posts. Making it an opt-in feature should help avoid any unexpected behavior. An alternative idea would be to enable listing archives by default, and post a very visible/urgent notice of breaking changes with the next release. 🙂

With listing archives disabled, a category assigned only to listing posts does not display content. Is this expected behavior? I'd expect that this type of category would display content regardless of my listing archive options.

So, this is a side effect of that decision to use post categories and tags. When viewing a category or tag term archive, WP doesn't really know whether the category or tag contains only listings. If listings aren't able to be shown in archives, then a category that contains only listings will indeed return 0 results. A category that contains both posts and listings will show the posts only. This does seem kind of wrong from a user perspective, but it's technically correct behavior, and I'm not sure of a better way to handle it. This is also status quo right now in the current release. Do you have an idea of how we might make this a bit less opaque to readers?

I suggest the archive options to be higher up on the page, right below the permalink inputs

Good idea! Updated in e7663b8.

@miguelpeixe
Copy link
Member

Thank you for the thorough explanations!

An alternative idea would be to enable listing archives by default, and post a very visible/urgent notice of breaking changes with the next release. 🙂

I can see a world where we'd have post type archive enabled but not necessarily categories. Can the "archive" option be divided into two options?

  • Enable listing post type archive
  • Enable listing categories archives

With that approach, we could have post type archive enabled and categories disabled. It wouldn't cause a change in how their sites are currently displaying content. What do you think?

With listing archives disabled, a category assigned only to listing posts does not display content. Is this expected behavior? I'd expect that this type of category would display content regardless of my listing archive options.

This does seem kind of wrong from a user perspective, but it's technically correct behavior, and I'm not sure of a better way to handle it. This is also status quo right now in the current release. Do you have an idea of how we might make this a bit less opaque to readers?

Makes sense, I just noticed that the strategy used on all_posts_are_type() handles only the current page for template definitions, which is reasonable.

The only solution I can think of is restoring custom taxonomies and have them both available, which doesn't sound awesome to me but at the same makes listing category navigation possible without it necessarily having to interfere with articles navigation.

I can file an issue so we can continue this discussion outside of this PR.

Good idea! Updated in e7663b8.

Thanks!

@dkoo
Copy link
Contributor Author

dkoo commented Sep 20, 2021

I can see a world where we'd have post type archive enabled but not necessarily categories. Can the "archive" option be divided into two options?

  • Enable listing post type archive
  • Enable listing categories archives

With that approach, we could have post type archive enabled and categories disabled. It wouldn't cause a change in how their sites are currently displaying content. What do you think?

I like this idea—thanks for the suggestion! Implemented in 26d5710.

@dkoo dkoo requested a review from miguelpeixe September 20, 2021 19:57
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Thank you for revising! :shipit:

@dkoo dkoo merged commit b2c3cc8 into master Sep 20, 2021
@dkoo dkoo deleted the feat/feature-extensions-from-gdg branch September 20, 2021 22:36
matticbot pushed a commit that referenced this pull request Sep 21, 2021
# [2.5.0-alpha.2](v2.5.0-alpha.1...v2.5.0-alpha.2) (2021-09-21)

### Bug Fixes

* editor crash when switching months in event dates block ([#123](#123)) ([cee0d53](cee0d53))
* fatal error when WooCommerce Subscriptions is not active ([#122](#122)) ([16ac166](16ac166))

### Features

* feature extensions from gdg ([#120](#120)) ([b2c3cc8](b2c3cc8))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.5.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Sep 21, 2021
# [2.5.0](v2.4.0...v2.5.0) (2021-09-21)

### Bug Fixes

* editor crash when switching months in event dates block ([#123](#123)) ([cee0d53](cee0d53))
* fatal error when WooCommerce Subscriptions is not active ([#122](#122)) ([16ac166](16ac166))

### Features

* feature extensions from gdg ([#120](#120)) ([b2c3cc8](b2c3cc8))
* **ugc:** automated WC products for self-serve listings ([#117](#117)) ([6a44d86](6a44d86))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants