From bc82031e65a4b8824560f17088d67224dfa053ea Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Tue, 28 Nov 2023 16:08:12 +0000 Subject: [PATCH 01/13] Use component wrapper on expander component - credit to @floehopper - relies upon https://github.com/alphagov/govuk_publishing_components/pull/3736 --- app/views/components/_expander.html.erb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/views/components/_expander.html.erb b/app/views/components/_expander.html.erb index 4d35bbd04..d142d39b0 100644 --- a/app/views/components/_expander.html.erb +++ b/app/views/components/_expander.html.erb @@ -7,11 +7,15 @@ content_id = "expander-content-#{SecureRandom.hex(4)}" shared_helper = GovukPublishingComponents::Presenters::SharedHelper.new(local_assigns) - css_classes = %w( app-c-expander ) - css_classes << (shared_helper.get_margin_bottom) unless margin_bottom == 0 + component_helper = GovukPublishingComponents::Presenters::ComponentWrapperHelper.new(local_assigns) + component_helper.add_data_attribute({ module: "expander" }) + component_helper.add_data_attribute({ "open-on-load": open_on_load }) + component_helper.add_data_attribute({ "ga4-filter-parent": title }) + component_helper.add_class("app-c-expander") + component_helper.add_class(shared_helper.get_margin_bottom) unless margin_bottom == 0 %> <% if title %> - <%= tag.div class: css_classes, data: { module: "expander", 'open-on-load': open_on_load, 'ga4-filter-parent': '' } do %> + <%= tag.div(**component_helper.all_attributes) do %>

<%= title %> From a5a7017284fc62812baf008d363d4fa1b770eb2b Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Wed, 29 Nov 2023 09:36:12 +0000 Subject: [PATCH 02/13] Set expander GA4 data attrs outside component - credit to @floehopper - decouples the component from this app without losing any of the existing GA4 tracking --- app/views/components/_date_filter.html.erb | 2 +- app/views/components/_expander.html.erb | 1 - app/views/finders/_taxon_facet.html.erb | 3 ++- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/components/_date_filter.html.erb b/app/views/components/_date_filter.html.erb index 3e8ac00e9..bb92e7ae6 100644 --- a/app/views/components/_date_filter.html.erb +++ b/app/views/components/_date_filter.html.erb @@ -8,7 +8,7 @@ date_errors_to ||= nil %> <% if key && name %> - <%= render "components/expander", { title: name } do %> + <%= render "components/expander", { title: name, data_attributes: { "ga4-filter-parent": name } } do %>
<%= render "govuk_publishing_components/components/input", { label: { diff --git a/app/views/components/_expander.html.erb b/app/views/components/_expander.html.erb index d142d39b0..81f72a241 100644 --- a/app/views/components/_expander.html.erb +++ b/app/views/components/_expander.html.erb @@ -10,7 +10,6 @@ component_helper = GovukPublishingComponents::Presenters::ComponentWrapperHelper.new(local_assigns) component_helper.add_data_attribute({ module: "expander" }) component_helper.add_data_attribute({ "open-on-load": open_on_load }) - component_helper.add_data_attribute({ "ga4-filter-parent": title }) component_helper.add_class("app-c-expander") component_helper.add_class(shared_helper.get_margin_bottom) unless margin_bottom == 0 %> diff --git a/app/views/finders/_taxon_facet.html.erb b/app/views/finders/_taxon_facet.html.erb index 1b3c6bab9..bf1d5d906 100644 --- a/app/views/finders/_taxon_facet.html.erb +++ b/app/views/finders/_taxon_facet.html.erb @@ -1,6 +1,7 @@ <% unless i_am_a_topic_page_finder %> <%= render "components/expander", { - title: "Topic" + title: "Topic", + data_attributes: { "ga4-filter-parent": "Topic" } } do %>
<%= render "govuk_publishing_components/components/select", { From ab361e5fccdeb94fb808bf688d5e49e15122242f Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Wed, 29 Nov 2023 09:40:11 +0000 Subject: [PATCH 03/13] Set date filter GA4 data attrs outside component - credit to @floehopper - this means more of the GA4 related data attributes for facets are now set in the relevant partial template, which makes the code easier to follow --- app/views/components/_date_filter.html.erb | 3 ++- app/views/finders/_date_facet.html.erb | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/views/components/_date_filter.html.erb b/app/views/components/_date_filter.html.erb index bb92e7ae6..d10be12d6 100644 --- a/app/views/components/_date_filter.html.erb +++ b/app/views/components/_date_filter.html.erb @@ -6,9 +6,10 @@ aria_controls_id ||= false date_errors_from ||= nil date_errors_to ||= nil + data_attributes ||= nil %> <% if key && name %> - <%= render "components/expander", { title: name, data_attributes: { "ga4-filter-parent": name } } do %> + <%= render "components/expander", { title: name, data_attributes: } do %>
<%= render "govuk_publishing_components/components/input", { label: { diff --git a/app/views/finders/_date_facet.html.erb b/app/views/finders/_date_facet.html.erb index 7137ba347..da7a54609 100644 --- a/app/views/finders/_date_facet.html.erb +++ b/app/views/finders/_date_facet.html.erb @@ -1,12 +1,12 @@ - <%= - render "components/date_filter", { + <%= render "components/date_filter", { name: date_facet.name, key: date_facet.key, from_value: date_facet.user_supplied_from_date, to_value: date_facet.user_supplied_to_date, aria_controls_id: "js-search-results-info", date_errors_to: date_facet.error_message_to(@search_query), - date_errors_from: date_facet.error_message_from(@search_query) + date_errors_from: date_facet.error_message_from(@search_query), + data_attributes: { "ga4-filter-parent": date_facet.name } } %> \ No newline at end of file From fd3aa5ffe70133c48e72d9c817ff51788db1436c Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Wed, 29 Nov 2023 09:44:56 +0000 Subject: [PATCH 04/13] Use TagHelper in facets - credit to @floehopper - GA4 data attributes should all now be implemented using a Ruby Hash, which will make setting it based on the facet object easier in a following commit --- app/views/finders/_option_select_facet.html.erb | 6 +++--- app/views/finders/_radio_facet.html.erb | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/finders/_option_select_facet.html.erb b/app/views/finders/_option_select_facet.html.erb index 14469d8fd..610b23bf9 100644 --- a/app/views/finders/_option_select_facet.html.erb +++ b/app/views/finders/_option_select_facet.html.erb @@ -1,4 +1,4 @@ -<%# +<%# Always ensure the option-select and checkbox stylesheets are requested, otherwise the helper methods to include the stylesheets are not called when caching is used @@ -7,7 +7,7 @@ <% add_gem_component_stylesheet("checkboxes") %> <% cache_if option_select_facet.cacheable?, option_select_facet.cache_key do %> -
+ <%= tag.div(data: { "ga4-change-category": "update-filter checkbox", "ga4-filter-parent": "", "ga4-section": option_select_facet.name }) do %> <%= render partial: 'govuk_publishing_components/components/option_select', locals: { :key => option_select_facet.key, :title => option_select_facet.name, @@ -19,5 +19,5 @@ :show_filter => option_select_facet.show_option_select_filter, :large => option_select_facet.large? } %> -
+ <% end %> <% end %> diff --git a/app/views/finders/_radio_facet.html.erb b/app/views/finders/_radio_facet.html.erb index 801275c9c..801aaed93 100644 --- a/app/views/finders/_radio_facet.html.erb +++ b/app/views/finders/_radio_facet.html.erb @@ -1,7 +1,7 @@ -
+<%= tag.div(class: "facet-container", data: { "ga4-change-category": "update-filter radio", "ga4-section": "", "ga4-filter-parent": "" }) do %> <%= render "govuk_publishing_components/components/radio", { name: radio_facet.key, small: true, items: radio_facet.options } %> -
+<% end %> From 5baa87878ea7b5a869cbe02f0e673b64f36e2340 Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Wed, 29 Nov 2023 13:58:23 +0000 Subject: [PATCH 05/13] Extract ga4_section onto facet subclasses - credit to @floehopper --- app/models/date_facet.rb | 4 ++++ app/models/facet.rb | 4 ++++ app/models/option_select_facet.rb | 4 ++++ app/models/radio_facet.rb | 4 ++++ app/models/radio_facet_for_multiple_filters.rb | 4 ++++ app/models/taxon_facet.rb | 4 ++++ app/models/topical_facet.rb | 4 ++++ app/views/finders/_date_facet.html.erb | 2 +- app/views/finders/_option_select_facet.html.erb | 2 +- app/views/finders/_radio_facet.html.erb | 2 +- app/views/finders/_taxon_facet.html.erb | 2 +- 11 files changed, 32 insertions(+), 4 deletions(-) diff --git a/app/models/date_facet.rb b/app/models/date_facet.rb index abb38d887..dd0ddadae 100644 --- a/app/models/date_facet.rb +++ b/app/models/date_facet.rb @@ -45,6 +45,10 @@ def query_params { key => date_values } end + def ga4_section + name + end + private def value_fragments diff --git a/app/models/facet.rb b/app/models/facet.rb index 5756a6fcb..0429ef555 100644 --- a/app/models/facet.rb +++ b/app/models/facet.rb @@ -55,6 +55,10 @@ def query_params {} end + def ga4_section + nil + end + private def and_word_connectors diff --git a/app/models/option_select_facet.rb b/app/models/option_select_facet.rb index c5f3f419a..2cf616162 100644 --- a/app/models/option_select_facet.rb +++ b/app/models/option_select_facet.rb @@ -72,6 +72,10 @@ def closed_on_load_mobile? open_on_load? end + def ga4_section + name + end + private def value_fragments diff --git a/app/models/radio_facet.rb b/app/models/radio_facet.rb index ce878518e..07546a9e1 100644 --- a/app/models/radio_facet.rb +++ b/app/models/radio_facet.rb @@ -30,6 +30,10 @@ def query_params { key => selected_value["value"] } end + def ga4_section + "" + end + private def selected_value diff --git a/app/models/radio_facet_for_multiple_filters.rb b/app/models/radio_facet_for_multiple_filters.rb index 41242e259..388931815 100644 --- a/app/models/radio_facet_for_multiple_filters.rb +++ b/app/models/radio_facet_for_multiple_filters.rb @@ -37,6 +37,10 @@ def allowed_values @filter_hashes end + def ga4_section + "" + end + private attr_reader :filter_hashes diff --git a/app/models/taxon_facet.rb b/app/models/taxon_facet.rb index 25c22f552..f7c1d6d86 100644 --- a/app/models/taxon_facet.rb +++ b/app/models/taxon_facet.rb @@ -43,6 +43,10 @@ def query_params } end + def ga4_section + "Topic" + end + private def value_fragments diff --git a/app/models/topical_facet.rb b/app/models/topical_facet.rb index 9d53f4ad8..6aff358ae 100644 --- a/app/models/topical_facet.rb +++ b/app/models/topical_facet.rb @@ -6,4 +6,8 @@ def allowed_values def to_partial_path "option_select_facet" end + + def ga4_section + name + end end diff --git a/app/views/finders/_date_facet.html.erb b/app/views/finders/_date_facet.html.erb index da7a54609..4e6904dd7 100644 --- a/app/views/finders/_date_facet.html.erb +++ b/app/views/finders/_date_facet.html.erb @@ -7,6 +7,6 @@ aria_controls_id: "js-search-results-info", date_errors_to: date_facet.error_message_to(@search_query), date_errors_from: date_facet.error_message_from(@search_query), - data_attributes: { "ga4-filter-parent": date_facet.name } + data_attributes: { "ga4-filter-parent": date_facet.ga4_section } } %> \ No newline at end of file diff --git a/app/views/finders/_option_select_facet.html.erb b/app/views/finders/_option_select_facet.html.erb index 610b23bf9..ec2127281 100644 --- a/app/views/finders/_option_select_facet.html.erb +++ b/app/views/finders/_option_select_facet.html.erb @@ -7,7 +7,7 @@ <% add_gem_component_stylesheet("checkboxes") %> <% cache_if option_select_facet.cacheable?, option_select_facet.cache_key do %> - <%= tag.div(data: { "ga4-change-category": "update-filter checkbox", "ga4-filter-parent": "", "ga4-section": option_select_facet.name }) do %> + <%= tag.div(data: { "ga4-change-category": "update-filter checkbox", "ga4-filter-parent": "", "ga4-section": option_select_facet.ga4_section }) do %> <%= render partial: 'govuk_publishing_components/components/option_select', locals: { :key => option_select_facet.key, :title => option_select_facet.name, diff --git a/app/views/finders/_radio_facet.html.erb b/app/views/finders/_radio_facet.html.erb index 801aaed93..14e4d79c1 100644 --- a/app/views/finders/_radio_facet.html.erb +++ b/app/views/finders/_radio_facet.html.erb @@ -1,4 +1,4 @@ -<%= tag.div(class: "facet-container", data: { "ga4-change-category": "update-filter radio", "ga4-section": "", "ga4-filter-parent": "" }) do %> +<%= tag.div(class: "facet-container", data: { "ga4-change-category": "update-filter radio", "ga4-section": "", "ga4-filter-parent": radio_facet.ga4_section }) do %> <%= render "govuk_publishing_components/components/radio", { name: radio_facet.key, small: true, diff --git a/app/views/finders/_taxon_facet.html.erb b/app/views/finders/_taxon_facet.html.erb index bf1d5d906..96d7f5b4b 100644 --- a/app/views/finders/_taxon_facet.html.erb +++ b/app/views/finders/_taxon_facet.html.erb @@ -1,7 +1,7 @@ <% unless i_am_a_topic_page_finder %> <%= render "components/expander", { title: "Topic", - data_attributes: { "ga4-filter-parent": "Topic" } + data_attributes: { "ga4-filter-parent": taxon_facet.ga4_section } } do %>
<%= render "govuk_publishing_components/components/select", { From 05d4a256b2aa5a88d03a41f52ce3060e3fb74384 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 20 Sep 2023 12:26:50 +0100 Subject: [PATCH 06/13] Render each facet individually vs as a collection This is effectively a more verbose version of the code it replaces. I want to pass another local variable to each facet partial and this change will make doing that easier. The `option_select_facet_counter` local variable was previously being provided automatically by Rails; now we're providing it explicitly. Previously Rails was providing a `*_counter` local variable to each facet partial where the prefix was based on the facet partial name. Now we're providing the *same* local variable to each facet partial and, although none of the other facet partials were using the counter, it makes sense to give it a more generic name - hence I've named it `index`. There _might_ be a more elegant way to do this, but I couldn't find it! The new version still relies on `FilterableFacet#to_partial_path`. As an aside I found it a bit confusing that this index is 0-indexed whereas the related index in the GOVUK.analyticsGa4.Ga4FinderTracker.setFilterIndexes() JS function [1] is 1-indexed. However, I haven't changed that here. [1]: https://github.com/alphagov/finder-frontend/blob/665e954f26da3aa288f4e516b0272eb07c7ead55/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js#L10-L25 --- app/views/finders/_facet_collection.html.erb | 4 +++- app/views/finders/_option_select_facet.html.erb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/views/finders/_facet_collection.html.erb b/app/views/finders/_facet_collection.html.erb index a60a97d4a..cb198a5ee 100644 --- a/app/views/finders/_facet_collection.html.erb +++ b/app/views/finders/_facet_collection.html.erb @@ -46,7 +46,9 @@
- <%= render facets.select(&:filterable?) %> + <% facets.select(&:filterable?).each.with_index do |facet, index| %> + <%= render partial: facet, object: facet, locals: { index: } %> + <% end %>
<%= render "facet_tags", facet_tags.present %>
diff --git a/app/views/finders/_option_select_facet.html.erb b/app/views/finders/_option_select_facet.html.erb index ec2127281..fe9144062 100644 --- a/app/views/finders/_option_select_facet.html.erb +++ b/app/views/finders/_option_select_facet.html.erb @@ -14,7 +14,7 @@ :aria_controls_id => "js-search-results-info", :options_container_id => option_select_facet.key, :options => option_select_facet.options("js-search-results-info", option_select_facet.key), - :closed_on_load => option_select_facet.closed_on_load?(option_select_facet_counter), + :closed_on_load => option_select_facet.closed_on_load?(index), :closed_on_load_mobile => option_select_facet.closed_on_load_mobile?, :show_filter => option_select_facet.show_option_select_filter, :large => option_select_facet.large? From 51ef79134a1bcab9bfa7de2b14aaaa07fb64e27d Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 20 Sep 2023 15:54:04 +0100 Subject: [PATCH 07/13] Reduce duplication in FindersController & facet_collection The facet_collection partial only ever uses the filterable facets, so we can avoid some duplication by selecting the filterable facets in FindersController#facets. However, there are a few callsites within the controller which use the full list of facets, so I've introduced a new `#all_facets` method to handle that without making it available as a view helper method. --- app/controllers/finders_controller.rb | 14 ++++++++++---- app/views/finders/_facet_collection.html.erb | 6 +++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/controllers/finders_controller.rb b/app/controllers/finders_controller.rb index bef8ceaa0..f02622148 100644 --- a/app/controllers/finders_controller.rb +++ b/app/controllers/finders_controller.rb @@ -92,7 +92,7 @@ def render_component(partial, locals) def result_set_presenter @result_set_presenter ||= ResultSetPresenter.new( content_item, - facets, + all_facets, results, filter_params, sort_presenter, @@ -106,12 +106,18 @@ def results @results ||= ResultSetParser.parse(search_results) end + def all_facets + return @facets if defined?(@facets) + + FacetsBuilder.new(content_item:, search_results:, value_hash: filter_params).facets + end + def facets - @facets ||= FacetsBuilder.new(content_item:, search_results:, value_hash: filter_params).facets + all_facets.select(&:filterable?) end def signup_links - @signup_links ||= SignupLinksPresenter.new(content_item, facets, keywords).signup_links + @signup_links ||= SignupLinksPresenter.new(content_item, all_facets, keywords).signup_links end def initialize_search_query(is_for_feed: false) @@ -174,7 +180,7 @@ def keywords def facet_tags @facet_tags ||= FacetTagsPresenter.new( - facets.select(&:filterable?), + facets, sort_presenter, i_am_a_topic_page_finder:, ) diff --git a/app/views/finders/_facet_collection.html.erb b/app/views/finders/_facet_collection.html.erb index cb198a5ee..f6cd3f521 100644 --- a/app/views/finders/_facet_collection.html.erb +++ b/app/views/finders/_facet_collection.html.erb @@ -18,12 +18,12 @@ <% if content_item.all_content_finder? %> <%= render partial: 'filter_button'%> <% elsif !content_item.all_content_finder? %> - <% if facets.select(&:filterable?).any? %> + <% if facets.any? %> <%= render partial: 'filter_button'%> <% end %> <% end %> - <% if facets.select(&:filterable?).any? %> + <% if facets.any? %>
<%= render "govuk_publishing_components/components/skip_link", { @@ -46,7 +46,7 @@
- <% facets.select(&:filterable?).each.with_index do |facet, index| %> + <% facets.each.with_index do |facet, index| %> <%= render partial: facet, object: facet, locals: { index: } %> <% end %>
From 69e149bcd6061fa1c42c29cca7ddee29323562b3 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 20 Sep 2023 16:34:29 +0100 Subject: [PATCH 08/13] WIP: Add ga4-index data attrs in Ruby vs JS We're now able to provide the index from the facet partials thus avoiding the need for the code in GOVUK.analyticsGa4.Ga4FinderTracker.setFilterIndexes(). However, I've left the code to trigger the "ga4-filter-indexes-added" event for now to reduce the scope of this change. The plan is that the whole of setFilterIndexes() will become redundant in a subsequent commit. Note that the index is 0-indexed vs 1-indexed since the former is how the Rails indexing was working. This means we currently have to add 1 in the facet partials when setting the ga4-index.index_section value. It might make more sense to change the index in FacetsIterator to be 1-indexed and change OptionSelectFacet#closed_by_default? to work with this. TODO: I'm not completely convinced that the Facet#has_ga4_section? method belongs in the model. --- .../analytics-ga4/ga4-finder-tracker.js | 13 ---------- app/controllers/finders_controller.rb | 2 +- app/lib/facets_iterator.rb | 14 +++++++++++ app/models/facet.rb | 4 +++ app/views/finders/_date_facet.html.erb | 2 +- app/views/finders/_facet_collection.html.erb | 4 +-- .../finders/_option_select_facet.html.erb | 25 +++++++++---------- app/views/finders/_radio_facet.html.erb | 2 +- app/views/finders/_taxon_facet.html.erb | 2 +- .../analytics-ga4/ga4-finder-tracker.spec.js | 20 --------------- 10 files changed, 36 insertions(+), 52 deletions(-) create mode 100644 app/lib/facets_iterator.rb diff --git a/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js b/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js index 4693fec0a..40890b4af 100644 --- a/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js +++ b/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js @@ -7,20 +7,7 @@ GOVUK.analyticsGa4.Ga4FinderTracker = { - // Finds the parent div containing the filters. Loops through each child div that has data-ga4-filter-parent on it . Sets an index on each of these child divs. setFilterIndexes: function () { - var filterContainer = document.querySelector('[data-ga4-filter-container]') - - if (!filterContainer) { - return - } - - var filterParents = filterContainer.querySelectorAll('[data-ga4-filter-parent]') - - for (var i = 0; i < filterParents.length; i++) { - filterParents[i].setAttribute('data-ga4-index', JSON.stringify({ index_section: i + 1, index_section_count: filterParents.length })) - } - window.GOVUK.triggerEvent(window, 'ga4-filter-indexes-added') }, diff --git a/app/controllers/finders_controller.rb b/app/controllers/finders_controller.rb index f02622148..044c1f6c1 100644 --- a/app/controllers/finders_controller.rb +++ b/app/controllers/finders_controller.rb @@ -113,7 +113,7 @@ def all_facets end def facets - all_facets.select(&:filterable?) + FacetsIterator.new(all_facets.select(&:filterable?)) end def signup_links diff --git a/app/lib/facets_iterator.rb b/app/lib/facets_iterator.rb new file mode 100644 index 000000000..01b035d02 --- /dev/null +++ b/app/lib/facets_iterator.rb @@ -0,0 +1,14 @@ +class FacetsIterator + delegate :select, :any?, to: :@facets + + def initialize(facets) + @facets = facets + @facets_with_ga4_section = @facets.select(&:has_ga4_section?) + end + + def each_with_index_and_count + @facets.each do |facet| + yield facet, @facets_with_ga4_section.index(facet), @facets_with_ga4_section.count + end + end +end diff --git a/app/models/facet.rb b/app/models/facet.rb index 0429ef555..a2a3a65f1 100644 --- a/app/models/facet.rb +++ b/app/models/facet.rb @@ -59,6 +59,10 @@ def ga4_section nil end + def has_ga4_section? + !ga4_section.nil? + end + private def and_word_connectors diff --git a/app/views/finders/_date_facet.html.erb b/app/views/finders/_date_facet.html.erb index 4e6904dd7..f1d33cad8 100644 --- a/app/views/finders/_date_facet.html.erb +++ b/app/views/finders/_date_facet.html.erb @@ -7,6 +7,6 @@ aria_controls_id: "js-search-results-info", date_errors_to: date_facet.error_message_to(@search_query), date_errors_from: date_facet.error_message_from(@search_query), - data_attributes: { "ga4-filter-parent": date_facet.ga4_section } + data_attributes: { "ga4-filter-parent": date_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } } } %> \ No newline at end of file diff --git a/app/views/finders/_facet_collection.html.erb b/app/views/finders/_facet_collection.html.erb index f6cd3f521..b86892cc3 100644 --- a/app/views/finders/_facet_collection.html.erb +++ b/app/views/finders/_facet_collection.html.erb @@ -46,8 +46,8 @@
- <% facets.each.with_index do |facet, index| %> - <%= render partial: facet, object: facet, locals: { index: } %> + <% facets.each_with_index_and_count do |facet, index, count| %> + <%= render partial: facet, object: facet, locals: { index:, count: } %> <% end %>
<%= render "facet_tags", facet_tags.present %> diff --git a/app/views/finders/_option_select_facet.html.erb b/app/views/finders/_option_select_facet.html.erb index fe9144062..f78484069 100644 --- a/app/views/finders/_option_select_facet.html.erb +++ b/app/views/finders/_option_select_facet.html.erb @@ -7,17 +7,16 @@ <% add_gem_component_stylesheet("checkboxes") %> <% cache_if option_select_facet.cacheable?, option_select_facet.cache_key do %> - <%= tag.div(data: { "ga4-change-category": "update-filter checkbox", "ga4-filter-parent": "", "ga4-section": option_select_facet.ga4_section }) do %> - <%= render partial: 'govuk_publishing_components/components/option_select', locals: { - :key => option_select_facet.key, - :title => option_select_facet.name, - :aria_controls_id => "js-search-results-info", - :options_container_id => option_select_facet.key, - :options => option_select_facet.options("js-search-results-info", option_select_facet.key), - :closed_on_load => option_select_facet.closed_on_load?(index), - :closed_on_load_mobile => option_select_facet.closed_on_load_mobile?, - :show_filter => option_select_facet.show_option_select_filter, - :large => option_select_facet.large? - } %> - <% end %> + <%= render partial: 'govuk_publishing_components/components/option_select', locals: { + :key => option_select_facet.key, + :title => option_select_facet.name, + :aria_controls_id => "js-search-results-info", + :options_container_id => option_select_facet.key, + :options => option_select_facet.options("js-search-results-info", option_select_facet.key), + :closed_on_load => option_select_facet.closed_on_load?(index), + :closed_on_load_mobile => option_select_facet.closed_on_load_mobile?, + :show_filter => option_select_facet.show_option_select_filter, + :large => option_select_facet.large?, + :data_attributes => { "ga4-change-category": "update-filter checkbox", "ga4-filter-parent": "", "ga4-section": option_select_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } } + } %> <% end %> diff --git a/app/views/finders/_radio_facet.html.erb b/app/views/finders/_radio_facet.html.erb index 14e4d79c1..cd21b9424 100644 --- a/app/views/finders/_radio_facet.html.erb +++ b/app/views/finders/_radio_facet.html.erb @@ -1,4 +1,4 @@ -<%= tag.div(class: "facet-container", data: { "ga4-change-category": "update-filter radio", "ga4-section": "", "ga4-filter-parent": radio_facet.ga4_section }) do %> +<%= tag.div(class: "facet-container", data: { "ga4-change-category": "update-filter radio", "ga4-filter-parent": radio_facet.ga4_section, "ga4-section": "", "ga4-index": { index_section: index + 1, index_section_count: count } }) do %> <%= render "govuk_publishing_components/components/radio", { name: radio_facet.key, small: true, diff --git a/app/views/finders/_taxon_facet.html.erb b/app/views/finders/_taxon_facet.html.erb index 96d7f5b4b..175094365 100644 --- a/app/views/finders/_taxon_facet.html.erb +++ b/app/views/finders/_taxon_facet.html.erb @@ -1,7 +1,7 @@ <% unless i_am_a_topic_page_finder %> <%= render "components/expander", { title: "Topic", - data_attributes: { "ga4-filter-parent": taxon_facet.ga4_section } + data_attributes: { "ga4-filter-parent": taxon_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } } } do %>
<%= render "govuk_publishing_components/components/select", { diff --git a/spec/javascripts/analytics-ga4/ga4-finder-tracker.spec.js b/spec/javascripts/analytics-ga4/ga4-finder-tracker.spec.js index c927a1f97..dbf39d2c2 100644 --- a/spec/javascripts/analytics-ga4/ga4-finder-tracker.spec.js +++ b/spec/javascripts/analytics-ga4/ga4-finder-tracker.spec.js @@ -349,26 +349,6 @@ describe('GA4 finder change tracker', function () { expect(window.dataLayer[0]).toEqual(expected) }) - it('sets the correct indexes', function () { - form.setAttribute('data-ga4-filter-container', '') - - for (var i = 0; i < 5; i++) { - var div = document.createElement('div') - div.setAttribute('data-ga4-filter-parent', '') - form.appendChild(div) - } - - // Grabs each data-ga4-filter-parent element within a data-ga4-filter-container, and sets the appropriate indexes. - window.GOVUK.analyticsGa4.Ga4FinderTracker.setFilterIndexes() - - var divs = form.querySelectorAll('[data-ga4-filter-parent]') - - for (i = 0; i < divs.length; i++) { - var expectedIndexObject = { index_section: i + 1, index_section_count: divs.length, index_link: undefined } - expect(divs[i].getAttribute('data-ga4-index')).toEqual(JSON.stringify(expectedIndexObject)) - } - }) - it('does nothing if the elementType or changeType does not exist', function () { inputParent = document.createElement('div') input = document.createElement('input') From 17aa7f134b8b47ec0d539a7b39f778c40e2b88f4 Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Wed, 6 Dec 2023 11:09:35 +0000 Subject: [PATCH 09/13] Change expander component - allow it to accept data attributes to be applied to the expand/collapse button - will allow us to add GA4 tracking via a Rails/component approach and remove some JS - likely to be a breaking change as will depend on some other stuff in a subsequent commit - brings the component more in line with the option select component changes coming here: https://github.com/alphagov/govuk_publishing_components/pull/3750 --- app/assets/javascripts/components/expander.js | 20 ++++++---- app/views/components/_expander.html.erb | 1 + app/views/components/docs/expander.yml | 40 +++++++++++++++++++ spec/components/expander_spec.rb | 16 ++++++++ spec/javascripts/components/expander-spec.js | 34 ++++++++++------ 5 files changed, 90 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/components/expander.js b/app/assets/javascripts/components/expander.js index 6e0cb9440..7a2812131 100644 --- a/app/assets/javascripts/components/expander.js +++ b/app/assets/javascripts/components/expander.js @@ -69,17 +69,21 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}; // if this ; is omitted, none $button.setAttribute('aria-expanded', expanded) $button.setAttribute('aria-controls', this.$content.getAttribute('id')) - // GA4 Accordion tracking. Relies on the ga4-finder-tracker setting the index first, so we wrap this in a custom event. - window.addEventListener('ga4-filter-indexes-added', function () { - if (window.GOVUK.analyticsGa4) { - if (window.GOVUK.analyticsGa4.Ga4FinderTracker) { - window.GOVUK.analyticsGa4.Ga4FinderTracker.addFilterButtonTracking($button, this.$toggle.innerHTML) + var buttonAttributes = this.$module.getAttribute('data-button-data-attributes') + if (buttonAttributes) { + try { + buttonAttributes = JSON.parse(buttonAttributes) + for (var rawKey in buttonAttributes) { + var key = rawKey.replace(/_/i, '-').toLowerCase() + var rawValue = buttonAttributes[rawKey] + var value = typeof rawValue === 'object' ? JSON.stringify(rawValue) : rawValue + $button.setAttribute('data-' + key, value) } + } catch (e) { + console.error('Error with expander button data attributes, invalid JSON passed' + e.message, window.location) } - }.bind(this)) - + } $button.innerHTML = toggleHtml - this.$toggle.parentNode.replaceChild($button, this.$toggle) } diff --git a/app/views/components/_expander.html.erb b/app/views/components/_expander.html.erb index 81f72a241..1ea33aa8f 100644 --- a/app/views/components/_expander.html.erb +++ b/app/views/components/_expander.html.erb @@ -12,6 +12,7 @@ component_helper.add_data_attribute({ "open-on-load": open_on_load }) component_helper.add_class("app-c-expander") component_helper.add_class(shared_helper.get_margin_bottom) unless margin_bottom == 0 + component_helper.add_data_attribute({ "button-data-attributes": button_data_attributes }) if local_assigns.include?(:button_data_attributes) %> <% if title %> <%= tag.div(**component_helper.all_attributes) do %> diff --git a/app/views/components/docs/expander.yml b/app/views/components/docs/expander.yml index cef3e760d..f13078ffa 100644 --- a/app/views/components/docs/expander.yml +++ b/app/views/components/docs/expander.yml @@ -28,3 +28,43 @@ examples: margin_bottom: 9 block: | This is some content that is passed to the component. It should be distinct from the component, in that the component should not style or interact with it, other than to show and hide it. + with_counter: + description: If there are form elements within the expander it can display a count of the number of form elements with a selected or input value. This is to make it appear the same as the option select component when appearing with it in search pages. Note that if any form elements are selected on page load, the component will expand by default. + data: + title: Things + block: | +
+ + +
+
+
+ + +
+
+
+ + +
+ with_button_data_attributes: + description: Allows data attributes to be passed to the component to be added to the expand/collapse button. The attributes are written to the parent element then read by the JavaScript and applied to the button. This is used for tracking purposes. + data: + title: Organisation + button_data_attributes: + ga4_expandable: "" + ga4_event: + event_name: "select_content" + type: "finder" + block: | + Sssh I'm hiding diff --git a/spec/components/expander_spec.rb b/spec/components/expander_spec.rb index 88965c2ac..cd948b838 100644 --- a/spec/components/expander_spec.rb +++ b/spec/components/expander_spec.rb @@ -42,4 +42,20 @@ def render_component(locals, &block) assert_select '.app-c-expander.govuk-\!-margin-bottom-9' end + + it "accepts button data attributes" do + button_attrs = { + ga4_expandable: "true", + ga4_event: { + event_name: "select_content", + type: "finder", + }, + } + + render_component(title: "Some title", button_data_attributes: button_attrs) do + "This is more info" + end + + assert_select ".app-c-expander[data-button-data-attributes='#{button_attrs.to_json}']" + end end diff --git a/spec/javascripts/components/expander-spec.js b/spec/javascripts/components/expander-spec.js index 6c31840c8..044e38415 100644 --- a/spec/javascripts/components/expander-spec.js +++ b/spec/javascripts/components/expander-spec.js @@ -126,30 +126,38 @@ describe('An expander module', function () { }) }) - describe('GA4 tracking', function () { + describe('adds data attributes to the button', function () { + var buttonAttrs = { + ga4_expandable: '', + ga4_event: { + event_name: 'select_content', + type: 'finder' + } + } + beforeEach(function () { $element = document.createElement('div') $element.innerHTML = html - - new GOVUK.Modules.Expander($element.querySelector('.app-c-expander')).init() + $element.querySelector('.app-c-expander').setAttribute('data-button-data-attributes', JSON.stringify(buttonAttrs)) }) afterEach(function () { $(document).off() }) - it('adds the ga4 event tracker values to the button', function () { + it('adds button data attributes passed to the component onto the button', function () { + new GOVUK.Modules.Expander($element.querySelector('.app-c-expander')).init() var $button = $($element).find('.app-c-expander__button') - window.GOVUK.triggerEvent(window, 'ga4-filter-indexes-added') - var expected = JSON.stringify({ - event_name: 'select_content', - type: 'finder', - section: 'Organisation', - index_section: 1, - index_section_count: 3 - }) - + var expected = JSON.stringify(buttonAttrs.ga4_event) + expect($button.attr('data-ga4-expandable')).toEqual('') expect($button.attr('data-ga4-event')).toEqual(expected) }) + + it('does not error with invalid button data attributes', function () { + $element.querySelector('.app-c-expander').setAttribute('data-button-data-attributes', 'invalidjson') + new GOVUK.Modules.Expander($element.querySelector('.app-c-expander')).init() + var $button = $($element).find('.app-c-expander__button') + expect($button.attr('data-ga4-expandable')).toEqual(undefined) + }) }) }) From b3a60ec8af465695fd8d8d41cd6ad2f089ae6b07 Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Wed, 6 Dec 2023 11:12:05 +0000 Subject: [PATCH 10/13] Pass GA4 tracking attributes to components - removes the need for some of the JS around setting indexes on JS generated elements --- .../analytics-ga4/ga4-finder-tracker.js | 30 ------------------- app/assets/javascripts/live_search.js | 16 ---------- app/views/components/_date_filter.html.erb | 3 +- app/views/finders/_date_facet.html.erb | 11 ++++++- .../finders/_option_select_facet.html.erb | 11 ++++++- app/views/finders/_taxon_facet.html.erb | 14 ++++++++- 6 files changed, 35 insertions(+), 50 deletions(-) diff --git a/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js b/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js index 40890b4af..789242922 100644 --- a/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js +++ b/app/assets/javascripts/analytics-ga4/ga4-finder-tracker.js @@ -6,11 +6,6 @@ GOVUK.analyticsGa4 = GOVUK.analyticsGa4 || {} GOVUK.analyticsGa4.Ga4FinderTracker = { - - setFilterIndexes: function () { - window.GOVUK.triggerEvent(window, 'ga4-filter-indexes-added') - }, - // Called when the search results updates. Takes the event target, and a string containing the type of change and element type. Creates the GTM schema and pushes it. // changeEventMetadata is a string referring to the type of form change and the element type that triggered it. For example 'update-filter checkbox'. trackChangeEvent: function (eventTarget, changeEventMetadata) { @@ -144,31 +139,6 @@ } catch (e) { console.error('GA4 configuration error: ' + e.message, window.location) } - }, - - addFilterButtonTracking: function (button, section) { - button.setAttribute('data-ga4-expandable', '') - var ga4JSON = { event_name: 'select_content', type: 'finder', section: section } - - try { - var index = button.closest('[data-ga4-index]') || undefined - if (index) { - var indexData = JSON.parse(index.getAttribute('data-ga4-index')) - // flatten the attributes in index into the main data - for (var prop in indexData) { - ga4JSON[prop] = indexData[prop] - } - } else { - console.warn('No data-ga4-index found on the following element or its parents:') - console.warn(button) - } - } catch (e) { - // if there's a problem with the index object, don't add the attribute - console.error('GA4 configuration error: ' + e.message, window.location) - return - } - - button.setAttribute('data-ga4-event', JSON.stringify(ga4JSON)) } } diff --git a/app/assets/javascripts/live_search.js b/app/assets/javascripts/live_search.js index f0bcc52ef..d11ae8b24 100644 --- a/app/assets/javascripts/live_search.js +++ b/app/assets/javascripts/live_search.js @@ -56,11 +56,9 @@ if (document.readyState === 'complete') { this.Ga4EcommerceTracking() - this.ga4SetFilterIndexes() } else { window.addEventListener('DOMContentLoaded', function () { this.Ga4EcommerceTracking() - this.ga4SetFilterIndexes() }.bind(this)) } @@ -634,19 +632,5 @@ } } - LiveSearch.prototype.ga4SetFilterIndexes = function () { - if (GOVUK.analyticsGa4 && GOVUK.analyticsGa4.Ga4FinderTracker) { - var consentCookie = GOVUK.getConsentCookie() - - if (consentCookie && consentCookie.settings) { - GOVUK.analyticsGa4.Ga4FinderTracker.setFilterIndexes() - } else { - window.addEventListener('cookie-consent', function () { - GOVUK.analyticsGa4.Ga4FinderTracker.setFilterIndexes() - }) - } - } - } - GOVUK.LiveSearch = LiveSearch }()) diff --git a/app/views/components/_date_filter.html.erb b/app/views/components/_date_filter.html.erb index d10be12d6..73d3d1426 100644 --- a/app/views/components/_date_filter.html.erb +++ b/app/views/components/_date_filter.html.erb @@ -7,9 +7,10 @@ date_errors_from ||= nil date_errors_to ||= nil data_attributes ||= nil + button_data_attributes ||= nil %> <% if key && name %> - <%= render "components/expander", { title: name, data_attributes: } do %> + <%= render "components/expander", { title: name, data_attributes:, button_data_attributes: } do %>
<%= render "govuk_publishing_components/components/input", { label: { diff --git a/app/views/finders/_date_facet.html.erb b/app/views/finders/_date_facet.html.erb index f1d33cad8..f54de961b 100644 --- a/app/views/finders/_date_facet.html.erb +++ b/app/views/finders/_date_facet.html.erb @@ -7,6 +7,15 @@ aria_controls_id: "js-search-results-info", date_errors_to: date_facet.error_message_to(@search_query), date_errors_from: date_facet.error_message_from(@search_query), - data_attributes: { "ga4-filter-parent": date_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } } + data_attributes: { "ga4-filter-parent": date_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } }, + button_data_attributes: { + ga4_expandable: "", + ga4_event: { + event_name: 'select_content', + type: 'finder', + section: date_facet.name, + index: { index_section: index + 1, index_section_count: count } + } + } } %> \ No newline at end of file diff --git a/app/views/finders/_option_select_facet.html.erb b/app/views/finders/_option_select_facet.html.erb index f78484069..3343688e5 100644 --- a/app/views/finders/_option_select_facet.html.erb +++ b/app/views/finders/_option_select_facet.html.erb @@ -17,6 +17,15 @@ :closed_on_load_mobile => option_select_facet.closed_on_load_mobile?, :show_filter => option_select_facet.show_option_select_filter, :large => option_select_facet.large?, - :data_attributes => { "ga4-change-category": "update-filter checkbox", "ga4-filter-parent": "", "ga4-section": option_select_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } } + :data_attributes => { "ga4-change-category": "update-filter checkbox", "ga4-filter-parent": "", "ga4-section": option_select_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } }, + :button_data_attributes => { + "ga4-expandable": "", + "ga4-event": { + event_name: 'select_content', + type: 'finder', + section: option_select_facet.name, + index: { index_section: index + 1, index_section_count: count } + } + } } %> <% end %> diff --git a/app/views/finders/_taxon_facet.html.erb b/app/views/finders/_taxon_facet.html.erb index 175094365..ed4edc3ef 100644 --- a/app/views/finders/_taxon_facet.html.erb +++ b/app/views/finders/_taxon_facet.html.erb @@ -1,7 +1,19 @@ <% unless i_am_a_topic_page_finder %> + <% + button_data_attributes = { + ga4_expandable: "", + ga4_event: { + event_name: 'select_content', + type: 'finder', + section: "Topic", + index: { index_section: index + 1, index_section_count: count } + } + } + %> <%= render "components/expander", { title: "Topic", - data_attributes: { "ga4-filter-parent": taxon_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } } + data_attributes: { "ga4-filter-parent": taxon_facet.ga4_section, "ga4-index": { index_section: index + 1, index_section_count: count } }, + button_data_attributes: } do %>
<%= render "govuk_publishing_components/components/select", { From 98ba0433776490ab190ebbee4a41495a592eab71 Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Tue, 12 Dec 2023 14:05:51 +0000 Subject: [PATCH 11/13] Bump components gem - includes the change to the option select component, which allows us to pass data attributes for the JS generated button --- Gemfile.lock | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6da17136f..f7cef132b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -181,7 +181,7 @@ GEM govuk_personalisation (0.15.0) plek (>= 1.9.0) rails (>= 6, < 8) - govuk_publishing_components (36.1.0) + govuk_publishing_components (37.0.0) govuk_app_config govuk_personalisation (>= 0.7.0) kramdown @@ -189,6 +189,7 @@ GEM rails (>= 6) rouge sprockets (>= 3) + sprockets-rails govuk_schemas (4.7.0) json-schema (>= 2.8, < 4.2) govuk_test (4.0.1) @@ -241,7 +242,7 @@ GEM msgpack (1.7.2) multi_test (1.1.0) mutex_m (0.2.0) - net-imap (0.4.7) + net-imap (0.4.8) date net-protocol net-pop (0.1.2) From 8bf942523f3bf0351ac9cfadbaf64eccb0469636 Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Tue, 12 Dec 2023 15:04:08 +0000 Subject: [PATCH 12/13] PR changes: expander JS replace all --- app/assets/javascripts/components/expander.js | 2 +- spec/javascripts/components/expander-spec.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/components/expander.js b/app/assets/javascripts/components/expander.js index 7a2812131..b7befeb53 100644 --- a/app/assets/javascripts/components/expander.js +++ b/app/assets/javascripts/components/expander.js @@ -74,7 +74,7 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}; // if this ; is omitted, none try { buttonAttributes = JSON.parse(buttonAttributes) for (var rawKey in buttonAttributes) { - var key = rawKey.replace(/_/i, '-').toLowerCase() + var key = rawKey.replace(/_/g, '-').toLowerCase() var rawValue = buttonAttributes[rawKey] var value = typeof rawValue === 'object' ? JSON.stringify(rawValue) : rawValue $button.setAttribute('data-' + key, value) diff --git a/spec/javascripts/components/expander-spec.js b/spec/javascripts/components/expander-spec.js index 044e38415..16060d543 100644 --- a/spec/javascripts/components/expander-spec.js +++ b/spec/javascripts/components/expander-spec.js @@ -131,7 +131,8 @@ describe('An expander module', function () { ga4_expandable: '', ga4_event: { event_name: 'select_content', - type: 'finder' + type: 'finder', + test_attribute_with_many_underscores: 'oh yes' } } From 5952026c7db64bc83cbc469c03afc2f3ad8c91d9 Mon Sep 17 00:00:00 2001 From: Andy Sellick Date: Wed, 13 Dec 2023 08:50:53 +0000 Subject: [PATCH 13/13] fixup --- spec/javascripts/components/expander-spec.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/spec/javascripts/components/expander-spec.js b/spec/javascripts/components/expander-spec.js index 16060d543..b32593467 100644 --- a/spec/javascripts/components/expander-spec.js +++ b/spec/javascripts/components/expander-spec.js @@ -128,11 +128,10 @@ describe('An expander module', function () { describe('adds data attributes to the button', function () { var buttonAttrs = { - ga4_expandable: '', + test_attribute_with_many_underscores: 'oh yes', ga4_event: { event_name: 'select_content', - type: 'finder', - test_attribute_with_many_underscores: 'oh yes' + type: 'finder' } } @@ -150,7 +149,7 @@ describe('An expander module', function () { new GOVUK.Modules.Expander($element.querySelector('.app-c-expander')).init() var $button = $($element).find('.app-c-expander__button') var expected = JSON.stringify(buttonAttrs.ga4_event) - expect($button.attr('data-ga4-expandable')).toEqual('') + expect($button.attr('data-test-attribute-with-many-underscores')).toEqual('oh yes') expect($button.attr('data-ga4-event')).toEqual(expected) }) @@ -158,7 +157,7 @@ describe('An expander module', function () { $element.querySelector('.app-c-expander').setAttribute('data-button-data-attributes', 'invalidjson') new GOVUK.Modules.Expander($element.querySelector('.app-c-expander')).init() var $button = $($element).find('.app-c-expander__button') - expect($button.attr('data-ga4-expandable')).toEqual(undefined) + expect($button.attr('data-test-attribute-with-many-underscores')).toEqual(undefined) }) }) })