-
Notifications
You must be signed in to change notification settings - Fork 6
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
Bump govuk_publishing_components from 35.15.2 to 35.16.0 #3165
Bump govuk_publishing_components from 35.15.2 to 35.16.0 #3165
Conversation
Bumps [govuk_publishing_components](https://github.com/alphagov/govuk_publishing_components) from 35.15.2 to 35.16.0. - [Changelog](https://github.com/alphagov/govuk_publishing_components/blob/main/CHANGELOG.md) - [Commits](alphagov/govuk_publishing_components@v35.15.2...v35.16.0) --- updated-dependencies: - dependency-name: govuk_publishing_components dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Follow these steps if you are doing a Rails upgrade. Search page examples to sanity check:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks fine - I've just added one comment that I think you have some duplicate stylesheet includes that can be removed (they are not causing any harm though as they are de-duped by the AssetHelper).
@@ -1,8 +1,8 @@ | |||
<% add_app_component_stylesheet("option-select") %> | |||
<% add_gem_component_stylesheet("option-select") %> | |||
<% add_gem_component_stylesheet("checkboxes") %> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be ok to delete both of these helpers add_gem_component_stylesheet("option-select")
and add_gem_component_stylesheet("checkboxes")
because the option-select
stylesheet is included here and checkboxes
stylesheet here.
I've double checked running locally, with these removed, and both stylesheets are still included:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jon-kirwan, have removed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andysellick - I think this will affect some of our GA4 work, I have thought of a solution though which is hopefully straightforward. Thanks
<% add_gem_component_stylesheet("checkboxes") %> | ||
|
||
<% cache_if option_select_facet.cacheable?, option_select_facet.cache_key do %> | ||
<%= render partial: 'components/option_select', locals: { | ||
<%= render partial: 'govuk_publishing_components/components/option_select', locals: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The finder-frontend
option-select
had these GA4 attributes on the parent<div>
:
data-ga4-change-category="update-filter checkbox" data-ga4-filter-parent
and on the child <div>
: data: { ga4_section: title }
Without these the GA4 tracking won't work for this component.
You should be able to wrap the render in just one <div>
with the attributes, e.g.
<div data-ga4-change-category="update-filter checkbox" data-ga4-filter-parent data-ga4-section="[The input label]">
And then when you add/remove a checkbox, there should be an event in the dataLayer.
I guess we have lost the functionality to test it though, unless you have a way to test this?
Thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this @AshGDS. I've applied your fix (in a separate PR) and raised another PR in the gem as these attributes had been incorrectly ported across when I moved the component there.
In terms of testing we might be able to check that the overall tracking for the option selects on this page is working in a feature test, but I think the JS tests for the filter parent/set index stuff should also be able to cover off a general test to check that the functionality works in principle (if they don't already - I suspect they do, I remember your tests being pretty thorough). I'll put a tech debt card on our board to investigate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andysellick Thanks 👍
a6a8740
to
ae3a880
Compare
- tracking for the option select component in finders depends upon some JS to set the right attributes - this wasn't working properly owing to the migration of the component, this fixes that
@AshGDS are you able to re-review? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, thought I had reviewed this yesterday. LGTM 👍
Bumps govuk_publishing_components from 35.15.2 to 35.16.0.
Changelog
Sourced from govuk_publishing_components's changelog.
Commits
e02ea96
Merge pull request #3624 from alphagov/release-35.16.033c68d3
Version 35.16.0b53cdcd
Merge pull request #3623 from alphagov/add-option-selectd8713ee
Update changelog8340558
Fix styling issue with input filter9e7536b
Fix incorrect marginf5427e4
Merge pull request #3614 from alphagov/dependabot/npm_and_yarn/jasmine-browse...ad7ef98
Bump jasmine-browser-runner from 2.2.0 to 2.3.0847137f
Merge pull request #3607 from alphagov/dependabot/npm_and_yarn/axe-core-4.8.1783d79e
Merge pull request #3608 from alphagov/dependabot/bundler/gds-api-adapters-91...Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditions
will show all of the ignore conditions of the specified dependency@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)