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

Facet weight #48

Merged
merged 11 commits into from
Nov 16, 2020
Merged

Facet weight #48

merged 11 commits into from
Nov 16, 2020

Conversation

Adnan-cds
Copy link
Contributor

@Adnan-cds Adnan-cds commented Nov 4, 2020

  • Added a "weight" field to both the localgov_directories_facets content entity and localgov_directories_facets_type config entity.
  • A facet sort plugin that respects the weight property of localgov_directoires_facets entities.
  • An update hook to "activate" the above.
  • Convinced our custom facet template preprocessor to respect the weight property of the localgov_directories_facets_type entities.

Improvement ideas

  • A Functional test to verify that the Facets are indeed rendered based on their weight. I will raise a separate ticket for this. This already exists thanks to @ekes

Resolves #35

The "weight" property is now an "entity key" of the localgov_directories_facets
content entity.  This allows Entity queries to sort it by the "weight" property.
Add the new "weight" field to both the localgov_directories_facets content
entity and localgov_directories_facets_type config entity.
The default value and the initial value of a Content entity field are treated
differently.  Only the initial value is saved in its database column.  This
impacts sorting behaviour of that field.
Copy link
Member

@stephen-cox stephen-cox left a comment

Choose a reason for hiding this comment

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

Hi @Adnan-cds

The code for this looks fine, but I have queries regarding the functionality.

It looks to be possible to order the facet types using a draggable interface but not the facet items, so to change these content editors will need to manually set this be editing the facet item. Is that correct? This is not a major problem as it would be quite a big overhaul of the facet item interface to implement a draggable way to set weights on these; although grouping facet items by their type would be a big improvement to the editor experience.

The main issue I find is the default behaviour when all the facet weights are set to 0. Before saving the facet type order these display on a directory channel page in a random order. Facet items are also randomly ordered. If content editors don't want to set the weight on these things I would expect them to display alphabetically. So there needs to be two sorts applied; weight and then alphabetical for equally weighted items.

If you can add this second sort then I'm happy to approve this.

@Adnan-cds
Copy link
Contributor Author

It looks to be possible to order the facet types using a draggable interface but not the facet items

That's right. Drupal core comes with a Draggable list class for config entities but not content entities. That stopped me from implementing the dragging functionality for the Facet items. It is still possible to achieve this through a custom list built on top of Drupal core's tabledrag functionality. That will be a UX improvement for editors but won't change the facet behaviour. Perhaps we can implement this as part of a separate ticket since it is not blocking the migration of Facet items.

If you can add this second sort then I'm happy to approve this.

Makes total sense. I shouldn't have missed this. I'll update the code and give you a shout.

Thanks for the feedback :)

Copy link
Member

@finnlewis finnlewis left a comment

Choose a reason for hiding this comment

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

Hi @Adnan-cds and @stephen-cox

I've tested too and can confirm the functionality works, even if it is all a bit of a UX challenge for site builders to change the ordering. Hopefully we can improve the UX in due course.

I imagine we will also need to extend the sorting options to for facets within a facet type to include default sorting by alphabetical, by weight and by number of results. The latter is a standard approach with faceted filters which I expect some people will want, such that large result sets can display facet options something like this:

Colour:

  • red (738)
  • blue (438)
  • green (37)

But that's for another PR.

I'm happy with this as is for now.

Thanks @Adnan-cds !

@finnlewis
Copy link
Member

Oh - I tested the hook_update_n too - installed the module, created some content and facets, then switched to this branch and ran updb. All seemed to work as expected.

@Adnan-cds
Copy link
Contributor Author

facets within a facet type to include default sorting by alphabetical

Thanks Finn. I hope to restore the alphabetical sorting (after weight-based sorting) by today. I will request you and Steve for another round of testing later.

@Adnan-cds Adnan-cds marked this pull request as draft November 6, 2020 13:48
Both Directory facet types and Directory facet items should be first sorted by
weight and then label.

In the Directory facet listing page, sorting happens based on Facet type,
weight, and then label.  This allows Facets of the same Facet type to all appear
together in this list.
@Adnan-cds
Copy link
Contributor Author

Ready for review once again. @stephen-cox @finnlewis

Both Directory facet types and Directory facet items should now be first sorted by weight and then label. This means both Facet types and Facet items with equal weights will be sorted by their labels.

In the Directory facet listing page (i.e. /admin/content/directories/facets), sorting now happens based on Facet type, next weight, and then label. This allows Facet items of the same Facet type to all appear together in this list. This is to improve editor experience as per Steve's suggestion.

@Adnan-cds Adnan-cds marked this pull request as ready for review November 9, 2020 18:21
Copy link
Member

@stephen-cox stephen-cox left a comment

Choose a reason for hiding this comment

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

Looks good @Adnan-cds Sorting is working as I would expect it; weight and then alphabetical. The change to the sort on the fact items page is also an improvement. Happy to approve this.

@Adnan-cds Adnan-cds merged commit 727bc11 into master Nov 16, 2020
@Adnan-cds
Copy link
Contributor Author

Thanks Steve, thanks Finn. I am merging this master.

@Adnan-cds Adnan-cds deleted the feature/facet-weight branch November 16, 2020 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort Facet items by weight
3 participants