Refactor of single page notification button #3073
Closed
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
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.
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) (cherry picked from commit dd0a733)
hannako
force-pushed
the
simplify_button_component
branch
from
dad6351 to
5e2119d
Compare
hannako
force-pushed
the
simplify_button_component
branch
from
5e2119d to
2cad684
Compare
hannako
changed the title
hannako
force-pushed
the
simplify_button_component
branch
from
2cad684 to
5a16f0d
Compare
AgaDufrat
approved these changes
Nov 18, 2022
hannako
force-pushed
the
simplify_button_component
branch
from
8c095e2 to
729ac90
Compare
hannako
force-pushed
the
simplify_button_component
branch
from
729ac90 to
d295aed
Compare
|
Superseded by #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.
Predominantly the work of @hannalaakso
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.NB: this will not be a breaking change, as we are using an attribute that is already being sent in the JSON response.
Why
We currently receive the entire notification button HTML from the Personalisation 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.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 personalisation 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