Set attributes for single page notification button based on Account API response #3071
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
hannalaakso
changed the title
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
ae14c55 to
61b1082
Compare
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
61b1082 to
42e9752
Compare
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
42e9752 to
ce4d1a5
Compare
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
ce4d1a5 to
2107a03
Compare
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
2107a03 to
dd0a733
Compare
We need to update the button text via an ajax call to the account API. Storing the button text as data attributes makes them available to the js function that is amended in the next commit.
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
dd0a733 to
447ddec
Compare
hannalaakso
changed the base branch from
custom_text_single_page_notification_button
to
main
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
447ddec to
a3fddf9
Compare
AgaDufrat
approved these changes
Nov 22, 2022
matthillco
reviewed
Nov 22, 2022
...assets/javascripts/govuk_publishing_components/components/single-page-notification-button.js
Outdated
Show resolved
Hide resolved
matthillco
reviewed
Nov 22, 2022
...assets/javascripts/govuk_publishing_components/components/single-page-notification-button.js
Outdated
Show resolved
Hide resolved
No longer replace the entire single page notification button HTML from the personalisation API response. Instead, check the boolean "active" in the response: if active is true, user has subscribed to notifications. Update the tracking attribute on the button HTML according to the value of active in the API response. Append the value of data-button-location, if it is provided in the markup, to the tracking attribute value. Update the button text if both data-button-text-unsubscribe and data-button-text-subscribe are provided in the markup attributes Add a new wrapper class .gem-c-single-page-notication-button__text to more reliably target the button text. Add tests to check whether the tracking attribute, including optional location, and the custom button text are updated correctly upon receiving the API response. Remove some no longer relevant test classes (.old-button-for-test and .new-button-for-test) since the whole button is no longer being replaced. Add an additional test for checking that the button is rendered visible when the API call completes (the button is initially hidden when JS runs)
hannalaakso
force-pushed
the
amend_js_for_custom_text_single_page_notification_button
branch
from
a3fddf9 to
209c34b
Compare
hannalaakso
changed the title
hannalaakso
deleted the
amend_js_for_custom_text_single_page_notification_button
branch
Merged
hannako
added a commit
to alphagov/account-api
that referenced
this pull request
Jan 6, 2023
This API endpoint is only used by the single page notification button component in the gem [1]. The gem has been updated to expect a different response [2]. 1 https://github.com/search?q=org%3Aalphagov+check-email-subscription&type=code 2 alphagov/govuk_publishing_components#3071
hannako
added a commit
to alphagov/account-api
that referenced
this pull request
Jan 6, 2023
This API endpoint is only used by the single page notification button component in the gem [1]. The gem has been updated to expect a different response [2]. 1 https://github.com/search?q=org%3Aalphagov+check-email-subscription&type=code 2 alphagov/govuk_publishing_components#3071
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.
Co-authored with @hannako
What
There is a tight coupling between the single page notification button component and the Account API which this PR unpicks. The account API is returning the full button HTML when there is a govuk_account session present. This makes it hard to extend the functionality of the component as any changes would need to be duplicated in account api.
How
This PR modifies the JavaScript that receives the API response to now only update the necessary button attributes instead of expecting to receive the entire button HTML and replacing the whole button. Specifically, the PR:
activein the API response (activeindicates whether the user has subscribed to the relevant notifications)data-button-location, if it is provided in the markup, to the above tracking attribute valuedata-button-text-unsubscribeanddata-button-text-subscribeare provided in the markup attributes.gem-c-single-page-notication-button__textto more reliably target the button text.old-button-for-testand.new-button-for-test) since the whole button is no longer being replaced.Why
We currently receive the entire notification button HTML from the Account API to replace the notification button markup rendered on page load - this is done to make the button reflect whether the user has subscribed to notifications for the particular topic or document collection.
This PR is in preparation for some later work to refactor the API. Once this PR has been merged, and we are no longer using the
button_locationorbutton_htmlfrom the API response, we will remove them from the API and only return theactiveflag.A note about testing
Both @hannako and I have tested the button to work correctly by running it locally via goverment-frontend with a GOV.UK Account.
Left to do later
Once the API response has been refactored on the backend to not return the entire button HTML, we should also remove passing the button location to the Account API in the JS and the corresponding test for it.
Visual changes
None
HTML for subscribe button
HTML for unsubscribe button
Trello https://trello.com/c/qjGJ7b5P/1507-set-tracking-attribute-and-location-for-the-single-page-notification-button-component-based-on-simplified-api-response