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

Fix/facets order #2288

Merged
merged 19 commits into from
Aug 23, 2021
Merged

Fix/facets order #2288

merged 19 commits into from
Aug 23, 2021

Conversation

oscarssanchez
Copy link
Contributor

Description of the Change

This PR fixes #2207

Currently, the problem seems to be https://github.com/10up/ElasticPress/blob/develop/includes/classes/Feature/Facets/Widget.php#L332 sorts alphabetically but doesn't have a logic to discern when ordering by count has been set. I'm adding two new optional parameters to the order_by_selected method so it accounts for this scenario.

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

@@ -332,7 +334,7 @@ private function order_by_selected( $terms, $selected_terms ) {
ksort( $selected_terms );
Copy link
Contributor

Choose a reason for hiding this comment

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

@oscarssanchez Maybe I'm missing something but it doesn't make sense to keep these ksort functions since we'll order the items late with uasort, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rahmon in this case i think it is worth keeping this when we are sorting by term name, but I forgot to account for ascending. I have added this here: 1b2c0a0 let me know your thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

@oscarssanchez When I'm trying to order by term name, the children terms are inverted.

For example setting this way Order Terms By: Term Name and Term Order: Descending, I have this:

  1. Cursus
  2. Consectetur
    1. xonsectetur children
    2. consectetur children

After selecting Consectetur (parent), the children appears that way:

  1. Consectetur
    1. consectetur children
    2. xonsectetur children

The same occurs when I select Term Order: Ascending.

However, it's working for Order Terms By: Count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Rahmon i have updated this per our meeting to do the correct ordering.

@Rahmon Rahmon assigned oscarssanchez and unassigned Rahmon Aug 9, 2021
@Rahmon Rahmon assigned oscarssanchez and unassigned Rahmon Aug 17, 2021
Rahmon
Rahmon previously approved these changes Aug 18, 2021
Copy link
Contributor

@Rahmon Rahmon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@felipeelia felipeelia added this to the 3.6.2 milestone Aug 19, 2021
@felipeelia
Copy link
Member

@Rahmon @oscarssanchez before we merge this one, a couple of questions:

  1. Are we fixing the problem for a +2 tree level? Like
Selected Term
   Child Term 1
     Grandchild Term 1
     Grandchild Term 2
   Child Term 2
Other term

  1. It seems now we have two different functions ordering trees of terms: Utils\get_term_tree() and Facets/Widget::order_by_selected() . Did you explore the possibility of having only one of them?

@oscarssanchez oscarssanchez requested a review from Rahmon August 23, 2021 16:12
@oscarssanchez oscarssanchez merged commit 1fda38a into develop Aug 23, 2021
@oscarssanchez oscarssanchez deleted the fix/facets-order branch August 23, 2021 16:14
felipeelia added a commit that referenced this pull request Aug 23, 2021
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.

BUG? Facets order after selection
3 participants