Change single page notification button allow list to an exemption list #2356
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
danacotoran
force-pushed
the
button-everywhere
branch
from
363eb70 to
8f5cb27
Compare
danacotoran
force-pushed
the
button-everywhere
branch
from
261a816 to
a3ce81e
Compare
danacotoran
changed the title
huwd
reviewed
Feb 22, 2022
huwd
requested changes
Feb 22, 2022
There was a problem hiding this comment.
Great work, really nice neat Ruby I think - and good catch on the flash and the test.
I wonder if we should add a test to catch the absence of the flash banner?
But generally this is a ✅
Gonna abuse the "request changes" feature to block any accidential merge of this until the 28th, when we've heard back from our last stakeholders.
danacotoran
force-pushed
the
button-everywhere
branch
5 times, most recently
from
95adc4f to
614f431
Compare
huwd
approved these changes
Mar 1, 2022
huwd
changed the title
This was missed in the previous PR which added the button to the consultation template. The success message is necessary to indicate when subscribing/unsubscribing to notifications has been completed Update test to verify that the success banner appears as expected on all publication types on which the single page notification button is present.
".app-c-published-dates:last-of-type" is failing because there is now another element after the published dates component.
Change single page notification button allow list to an exemption list. Add tests to verify that pages whose content item is on the exemption list do not display with the single page notification button. Ensure that the button appears only on detailed_guides, publications and consultations and update tests for all other types of publications to check that the button is not included on those types.
danacotoran
force-pushed
the
button-everywhere
branch
from
4807367 to
6ecd50c
Compare
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.
The next stage for the single page notification rollout is to put the button on all pages with a
publication,detailed_guideorconsultationtype.The exemption mechanism will be a denylist. Publishers can request that the button not be included on certain individual pages. The way the exemption is applied is by adding the
content_idof the page to the single page notification button exclusion list.Implement notification button exemption list:
Change single page notification button allow list to an exemption list.
Switch from using
base_paths tocontent_ids for long term stability (content ids persist, base paths can change)Add some basic tests to verify that pages whose content item is on the exemption list do not display with the single page notification button.
Put account flash message on
consultation:This was missed in the previous PR which added the button to the
consultationtemplate. The success message is necessary to indicate when subscribing/unsubscribing to notifications has been completed.Fix failing test:
.app-c-published-dates:last-of-typeis failing because there is now another element after the published dates component.The
:last-of-typeselector doesn't work the way one might assume on first glance, as in it doesn't apply to classes the same way that it applies to element selectors.https://stackoverflow.com/questions/13211453/css-how-to-say-classlast-of-type-classes-not-elements
https://trello.com/c/4OWUb0no