From 5789c5ad2ed18a1a2528363c95eb0fd82a7af803 Mon Sep 17 00:00:00 2001 From: Dana Cotoran Date: Wed, 24 Nov 2021 10:13:50 +0000 Subject: [PATCH 1/3] Fix single page notification button data attrs The data attributes for tracking are not named correctly. data-label should be data-track-label, data-action should be data-track-action, data-category should be data-track-category. --- .../single_page_notification_button_helper.rb | 6 ++--- .../single_page_notification_button_spec.rb | 24 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb b/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb index 7b8c8b9372..46b89abdf7 100644 --- a/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb +++ b/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb @@ -17,12 +17,12 @@ def data module_names = %w[gem-track-click] module_names << "single-page-notification-button" if js_enhancement - @data_attributes[:label] = base_path + @data_attributes[:track_label] = base_path # data-action for tracking should have the format of e.g. "Unsubscribe-button-top", or "Subscribe-button-bottom" # when button_location is not present data-action will fall back to "Unsubscribe-button"/"Subscribe-button" - @data_attributes[:action] = [button_type, "button", button_location].compact.join("-") + @data_attributes[:track_action] = [button_type, "button", button_location].compact.join("-") @data_attributes[:module] = module_names.join(" ") - @data_attributes[:category] = "Single-page-notification-button" + @data_attributes[:track_category] = "Single-page-notification-button" # This attribute is passed through to the personalisation API to ensure when a new button is returned from the API, it has the same button_location @data_attributes[:button_location] = button_location @data_attributes diff --git a/spec/components/single_page_notification_button_spec.rb b/spec/components/single_page_notification_button_spec.rb index e7037c01b9..39e74f807b 100644 --- a/spec/components/single_page_notification_button_spec.rb +++ b/spec/components/single_page_notification_button_spec.rb @@ -51,42 +51,42 @@ def component_name it "has correct attributes for tracking by default" do render_component({ base_path: "/the-current-page" }) - assert_select ".gem-c-single-page-notification-button[data-category='Single-page-notification-button'][data-action='Subscribe-button'][data-label='/the-current-page']" + assert_select ".gem-c-single-page-notification-button[data-track-category='Single-page-notification-button'][data-track-action='Subscribe-button'][data-track-label='/the-current-page']" end it "has correct attributes for tracking when already_subscribed is true" do render_component({ base_path: "/the-current-page", already_subscribed: true }) - assert_select ".gem-c-single-page-notification-button[data-category='Single-page-notification-button'][data-action='Unsubscribe-button'][data-label='/the-current-page']" + assert_select ".gem-c-single-page-notification-button[data-track-category='Single-page-notification-button'][data-track-action='Unsubscribe-button'][data-track-label='/the-current-page']" end - it "has the correct default data-action for tracking when button_location is top" do + it "has the correct default data-track-action for tracking when button_location is top" do render_component({ base_path: "/the-current-page", button_location: "top" }) - assert_select ".gem-c-single-page-notification-button[data-action='Subscribe-button-top']" + assert_select ".gem-c-single-page-notification-button[data-track-action='Subscribe-button-top']" end - it "has the correct data-action for tracking when button_location is top and already_subscribed is true" do + it "has the correct data-track-action for tracking when button_location is top and already_subscribed is true" do render_component({ base_path: "/the-current-page", button_location: "top", already_subscribed: true }) - assert_select ".gem-c-single-page-notification-button[data-action='Unsubscribe-button-top']" + assert_select ".gem-c-single-page-notification-button[data-track-action='Unsubscribe-button-top']" end - it "has the correct default data-action for tracking when button_location is bottom" do + it "has the correct default data-track-action for tracking when button_location is bottom" do render_component({ base_path: "/the-current-page", button_location: "bottom" }) - assert_select ".gem-c-single-page-notification-button[data-action='Subscribe-button-bottom']" + assert_select ".gem-c-single-page-notification-button[data-track-action='Subscribe-button-bottom']" end - it "has the correct data-action for tracking when button_location is bottom and already_subscribed is true" do + it "has the correct data-track-action for tracking when button_location is bottom and already_subscribed is true" do render_component({ base_path: "/the-current-page", button_location: "bottom", already_subscribed: true }) - assert_select ".gem-c-single-page-notification-button[data-action='Unsubscribe-button-bottom']" + assert_select ".gem-c-single-page-notification-button[data-track-action='Unsubscribe-button-bottom']" end - it "has the correct data-action for tracking when button_location has an invalid value" do + it "has the correct data-track-action for tracking when button_location has an invalid value" do render_component({ base_path: "/the-current-page", button_location: "this is unacceptable" }) - assert_select ".gem-c-single-page-notification-button[data-action='Subscribe-button']" + assert_select ".gem-c-single-page-notification-button[data-track-action='Subscribe-button']" end end From 649506761bfdba6d30fb6c885da36b1bc18fae4b Mon Sep 17 00:00:00 2001 From: Dana Cotoran Date: Wed, 24 Nov 2021 13:52:56 +0000 Subject: [PATCH 2/3] update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91178d86fd..e6ebf0d542 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ * Update search component ([PR #2462](https://github.com/alphagov/govuk_publishing_components/pull/2462)) * Fix link to Crown Copyright in footer ([PR #2475](https://github.com/alphagov/govuk_publishing_components/pull/2475)) +* Fix single page notification button data attributes for tracking ([PR #2471](https://github.com/alphagov/govuk_publishing_components/pull/2471)) ## 27.14.1 From 2dc71b5e11f5ced95d194d22283989782e7ad098 Mon Sep 17 00:00:00 2001 From: Dana Cotoran Date: Wed, 24 Nov 2021 18:21:13 +0000 Subject: [PATCH 3/3] Apply track click attribute to wrapper Apply the data-module="gem-track-click" for the single page notification button to a component wrapper div. The reason behind applying this to a wrapper and not to the form is that the form gets swapped out with the form returned from the personalisation endpoint, so the track click module would have to be re-started on the new, updated form. Applying the track-click attribute to a wrapper instead means we don't have to re-start the gem-track-click module for the updated form. --- .../single-page-notification-button.js | 6 +++--- .../_single_page_notification_button.html.erb | 16 +++++++++------- .../single_page_notification_button_helper.rb | 5 +---- .../single_page_notification_button_spec.rb | 16 +++++++++------- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/govuk_publishing_components/components/single-page-notification-button.js b/app/assets/javascripts/govuk_publishing_components/components/single-page-notification-button.js index 4f6aad9904..a5d1148859 100644 --- a/app/assets/javascripts/govuk_publishing_components/components/single-page-notification-button.js +++ b/app/assets/javascripts/govuk_publishing_components/components/single-page-notification-button.js @@ -26,9 +26,9 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}; var html = document.createElement('div') html.innerHTML = newButton // test that the html returned contains the button component; if yes, swap the button for the updated version - var responseHasButton = html.querySelector('form.gem-c-single-page-notification-button .gem-c-single-page-notification-button__submit') - if (responseHasButton) { - this.$module.outerHTML = newButton + var responseButtonContainer = html.querySelector('form.gem-c-single-page-notification-button') + if (responseButtonContainer) { + this.$module.parentNode.replaceChild(responseButtonContainer, this.$module) } } } diff --git a/app/views/govuk_publishing_components/components/_single_page_notification_button.html.erb b/app/views/govuk_publishing_components/components/_single_page_notification_button.html.erb index c0aedc7557..81f991b774 100644 --- a/app/views/govuk_publishing_components/components/_single_page_notification_button.html.erb +++ b/app/views/govuk_publishing_components/components/_single_page_notification_button.html.erb @@ -2,16 +2,18 @@ component_helper = GovukPublishingComponents::Presenters::SinglePageNotificationButtonHelper.new(local_assigns) shared_helper = GovukPublishingComponents::Presenters::SharedHelper.new(local_assigns) - wrapper_classes = %w(gem-c-single-page-notification-button govuk-!-display-none-print) + wrapper_classes = %w(govuk-!-display-none-print) wrapper_classes << shared_helper.get_margin_bottom %> <% button_text = capture do %> <%= component_helper.button_text %> <% end %> -<%= tag.form class: wrapper_classes, action: "/email/subscriptions/single-page/new", method: "POST", data: component_helper.data do %> - - <%= content_tag(:button, button_text, { - class: "govuk-body-s gem-c-single-page-notification-button__submit", - type: "submit", - }) %> +<%= tag.div class: wrapper_classes, data: { module: "gem-track-click"} do %> + <%= tag.form class: "gem-c-single-page-notification-button", action: "/email/subscriptions/single-page/new", method: "POST", data: component_helper.data do %> + + <%= content_tag(:button, button_text, { + class: "govuk-body-s gem-c-single-page-notification-button__submit", + type: "submit", + }) %> + <% end %> <% end if component_helper.base_path %> diff --git a/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb b/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb index 46b89abdf7..f031f73ae7 100644 --- a/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb +++ b/lib/govuk_publishing_components/presenters/single_page_notification_button_helper.rb @@ -14,14 +14,11 @@ def initialize(local_assigns) end def data - module_names = %w[gem-track-click] - module_names << "single-page-notification-button" if js_enhancement - @data_attributes[:track_label] = base_path # data-action for tracking should have the format of e.g. "Unsubscribe-button-top", or "Subscribe-button-bottom" # when button_location is not present data-action will fall back to "Unsubscribe-button"/"Subscribe-button" @data_attributes[:track_action] = [button_type, "button", button_location].compact.join("-") - @data_attributes[:module] = module_names.join(" ") + @data_attributes[:module] = "single-page-notification-button" if js_enhancement @data_attributes[:track_category] = "Single-page-notification-button" # This attribute is passed through to the personalisation API to ensure when a new button is returned from the API, it has the same button_location @data_attributes[:button_location] = button_location diff --git a/spec/components/single_page_notification_button_spec.rb b/spec/components/single_page_notification_button_spec.rb index 39e74f807b..77ba69d98a 100644 --- a/spec/components/single_page_notification_button_spec.rb +++ b/spec/components/single_page_notification_button_spec.rb @@ -31,22 +31,24 @@ def component_name assert_select ".gem-c-single-page-notification-button[data-custom-attribute='kaboom!']" end - it "sets a default bottom margin" do + it "sets a default bottom margin to its wrapper" do render_component({ base_path: "/the-current-page" }) - assert_select '.gem-c-single-page-notification-button.govuk-\!-margin-bottom-3' + assert_select 'div.govuk-\!-margin-bottom-3 .gem-c-single-page-notification-button' end - it "adds bottom margin if margin_bottom is specified" do + it "adds bottom margin to its wrapper if margin_bottom is specified" do render_component({ base_path: "/the-current-page", margin_bottom: 9 }) - assert_select '.gem-c-single-page-notification-button.govuk-\!-margin-bottom-9' + assert_select 'div.govuk-\!-margin-bottom-9 .gem-c-single-page-notification-button' end it "has a data-module attribute for JavaScript, if the js-enhancement flag is present" do render_component({ base_path: "/the-current-page", js_enhancement: true }) - dom = Nokogiri::HTML(rendered) - form_data_module = dom.xpath("//form")[0].attr("data-module") + assert_select ".gem-c-single-page-notification-button[data-module='single-page-notification-button']" + end - expect(form_data_module).to include("single-page-notification-button") + it "does not have a data-module attribute if the js-enhancement flag is not present" do + render_component({ base_path: "/the-current-page" }) + assert_select ".gem-c-single-page-notification-button[data-module='single-page-notification-button']", false end it "has correct attributes for tracking by default" do