Spike: Refactor option-select component to isolate it from GA4 functionality specific to finder-frontend #3166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This array of CSS classes is actually applied to the options container within the component and not to the top-level element. Also govuk_publishing_components components typically allow the CSS classes on the top-level element to be customised via a `classes` local variable, so I think the new name is less confusing.
I think it's simpler and more idiomatic to use the GovukPublishingComponents::Presenters::ComponentWrapperHelper to build the top-level element. Also it means the data attributes on the top-level element can now easily be customised. I'm planning to move the setting of the ga4-related data attributes out of this template and this change will facilitate that. For the moment I haven't used ComponentWrapperHelper#add_class to add the "app-c-option-select" CSS class to the top-level element, because the validation only allows the "gem-c-*" prefix; not the "app-c-*" prefix. I plan to address this once the component is extracted into GovukPublishingComponents which will then make it possible to customise the CSS classes on the top-level element.
I want to extract the option-select component into the govuk_publishing_components gem so that we can use it in the signon application without duplicating the code. The signon app doesn't yet fully support GA4 and we don't have any immediate need to add GA tracking to our instances of option-select. Even if we did, the tracking added here feels specific to finder-frontend and so not suitable for extraction into the shared gem. Setting the GA4-related data attributes outside the component decouples the component from finder-frontend without losing any of the existing GA4 tracking. Also the e.g. the `ga4-change-category` data attribute is set in other facet partials rather than in individual components, so this brings the option-select facet into line with those.
I think it's simpler and more idiomatic to use the GovukPublishingComponents::Presenters::ComponentWrapperHelper to build the top-level element. Also it means the data attributes on the top-level element can now easily be customised. I'm planning to move the setting of the ga4-related data attributes out of this template and this change will facilitate that. For the moment I haven't used ComponentWrapperHelper#add_class to add the "app-c-expander" CSS class to the top-level element, because the validation only allows the "gem-c-*" prefix; not the "app-c-*" prefix. I plan to address this once the component is extracted into GovukPublishingComponents which will then make it possible to customise the CSS classes on the top-level element.
I did something similar to the option-select component in a previous commit. This brings the expander component into line with that change. Setting the GA4-related data attributes outside the component decouples the component from finder-frontend without losing any of the existing GA4 tracking.
This means that more of the GA4-related data attributes for facets are now set in the relevant facet partial template which I think makes the code easier to follow.
This means that all the places where the "ga4-section" data attribute is set are implemented using a Ruby Hash. I'm planning to set this value based on the relevant facet object in a subsequent commit. This change should make that easier.
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
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.
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.
This data attribute doesn't seem to depend on anything else so it's simpler to move it into the relevant components at the cost of a small amount of duplication. This is a small step towards making addFilterButtonTracking() redundant. Note that I may move the setting of this data attribute out into the facet partials in a subsequent commit.
As opposed to in option-select.js or expander.js thereby avoiding the need for GOVUK.analyticsGa4.Ga4FinderTracker.setFilterIndexes() to trigger the "ga4-filter-indexes-added" custom event and the need to call GOVUK.analyticsGa4.Ga4FinderTracker.addFilterButtonTracking() from option-select.js & expander.js. Now the ga4-related data attributes to be attached to the button created dynamically by the components are supplied in the `button_data_attributes` local variable in the relevant facet partial. This is more awkward than I would've liked, because of the need to make the button data attributes available at component JS initialization time as opposed to just at component partial render time. The awkwardness described above makes me think it would be worth investigating some kind of middle ground where _some_ of the GA4 functionality is optionally enabled within the components but the relevant distinguishing variables are still set in the facet partials, i.e. it's probably OK for the components to "know" that they should set `event_name` to "select_content" in the "ga4-event" data attribute, but it's probably not OK for the components to "know" that they should set `type` to "finder", so the latter should be passed in to the component from the facet partial. TODO: * Make equivalent changes to the expander component and all the facet partials which directly or indirectly use it. This may be a bit more awkward! * Consider making the index from FacetsIterator 1-indexed rather than 0-indexed so we don't have to keep adding 1 in several places. * Consier reducing duplication in the generation of the data attributes in the facet partials; perhaps by extracting a helper method. * Completely remove setFilterIndexes() & addFilterButtonTracking() * Do we still need to set the ga4-index data attribute on the component top-level element or was this only used by the component JS to obtain the ga4-index data? It would simplify the code in the facet partials if we only needed to pass the ga4-index data in the `button_data_attributes` and not in the `data_attributes` as well. * Update docs/analytics-ga4/ga4-finder-tracker.md & docs/analytics-ga4/ga4-filter-expand-collapse.md docs to reflect the new approach.
Merged
|
Relevant improvements from this PR have been implemented in this PR: #3224 so closing this one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/qs60lCxl
This is a spike into moving the GA4-related code out of the
option-selectcomponent prior to extracting it into thegovuk_publishing_componentsgem. My motivation was that I felt the GA4-related code was quite specific to finder-frontend.Broadly the approach I've tried to take is to move the setting of all GA4-related data attributes out into the Rails facet templates rather than relying on setting them in a combination of in the component templates, in the component JavaScript, and in some more "global" GA4 JavaScript. This branch is very much a spike so I've made no attempt at fixing any linting or tests and while I've tried hard to make the commit notes as detailed and useful as possible, the last few are marked as "WIP" because either I'm not very happy with them or there is still some work to do in them.
I think my conclusion is that, while this specific approach might not be the best, it might be worth going for some kind of middle ground where it's possible to enable GA4-related functionality within the components, but supply non-generic values as data attributes from the facet templates. I've tried to explain this in a bit more detail in the final commit note.
I'm very happy to chat about any of this if it would be useful! Otherwise I'll probably close the PR fairly soon.
/cc @andysellick