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

Change finder presenter logic for nested facets #3028

Merged

Conversation

lauraghiorghisor-tw
Copy link
Contributor

@lauraghiorghisor-tw lauraghiorghisor-tw commented Feb 25, 2025

Changing nested facets implementation. We now send two distinct facets in the publishing-api load, corresponding to the main and sub facet, respectively, in order to minimise how much we need to change Finder Frontend down the line.

Trello

@minhngocd minhngocd force-pushed the change-finder-presenter-logic-for-nested-facets branch from 2f822d6 to be536c4 Compare February 25, 2025 12:41
@@ -27,14 +27,6 @@ def content_id
file.fetch("content_id")
end

def self.facets_without_specialist_publisher_properties(facets)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to do the moving of this into FinderFacetPresenter in a separate commit to then adding the sub_facet_hash logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated now. The refactoring is in a separate commit

@minhngocd minhngocd force-pushed the change-finder-presenter-logic-for-nested-facets branch from be536c4 to 086b703 Compare February 25, 2025 17:53
minhngocd and others added 3 commits February 26, 2025 09:31
…nter

There is going to be additional logic we want to add into finder facets, which makes sense for us to pull into a separate object.
The new object allows us to remove the static method currently there, and simplifies testing for facets.
…ishing-api

In order to minimise the customisation in Finder Frontend, we will send the data in the shape it is typically expected by Finder Frontend, i.e. the facets are stand-alone. We still need some way of referencing the parent facet value in the children, which we're doing by setting `main_facet_value`, alongside every subfacet label-value pair in the subfacet. We also send the `main_facet_label` so we can recompose the subfacet label in the format `Main facet label - Sub facet label` when JS is disabled and all the facet options show in the select; since we have duplicates such as "Not applicable" it would be unclear which main facet option the subfacet option refers to.

Other required keys are:
- the `nested_facet` (T/F) flag which indicates to the Finder Frontend rendering app that the facet should be rendered as a NestedFacet (new Facet type added by us).
- `sub_facet_key` and `sub_facet_name` which are only set on the main facet, used for loading the js module.
- the `key` and `name` of the subfacet come from the data stored under the main facet, in the schema.
- all the other key values are shared between the main and sub facets, though a case could be made in the future for also allowing the customization of the subfacet's `short_name`, `preposition`, `display_as_result_metadata` or `show_option_select_filter` values.

Co-authored-by: Laura Ghiorghisor <laura.ghiorghisor@digital.cabinet-office.gov.uk>
@lauraghiorghisor-tw lauraghiorghisor-tw force-pushed the change-finder-presenter-logic-for-nested-facets branch from 086b703 to a97cb65 Compare February 26, 2025 09:31
@lauraghiorghisor-tw lauraghiorghisor-tw force-pushed the change-finder-presenter-logic-for-nested-facets branch from a97cb65 to 5fd9e7c Compare February 26, 2025 15:26
@lauraghiorghisor-tw lauraghiorghisor-tw merged commit b59a8c1 into main Feb 26, 2025
11 checks passed
@lauraghiorghisor-tw lauraghiorghisor-tw deleted the change-finder-presenter-logic-for-nested-facets branch February 26, 2025 17:06
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.

3 participants