Refactor Brexit callout #1958
Merged
Refactor Brexit callout #1958
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
Previously we introduced this notice to inform users of a "no-deal" outcome, but it has since been re-used for a "deal". While we can't easily change the attributes in the content item, we can at least fix the inaccurate terminology within this app.
benthorner
force-pushed
the
refactor-no-deal
branch
from
8406ead to
8f931a8
Compare
Previously we had a generic "header notice" component that was only actually used once. While we should aim for generic components, the behaviour of this one is clearly coupled to the needs of Brexit. By clearly marking it as a "brexit thing", we: - Make it easier to identify for removal, along with other bits of code that were only added around the transition period. - Make it more flexible, without having to worry about accomodating other use cases (since there are none).
benthorner
force-pushed
the
refactor-no-deal
branch
from
8f931a8 to
a494556
Compare
huwd
reviewed
Dec 30, 2020
| module NoDealNotice | ||
| def has_no_deal_notice? | ||
| module BrexitNotice | ||
| def has_brexit_notice? | ||
| content_item.dig("details").key?("brexit_no_deal_notice") |
There was a problem hiding this comment.
mmm yeah, that's a good reminder for future eh?
overly descriptive names for things that might get hijacked in future!
There was a problem hiding this comment.
Yeah, it's an interesting one: going in it may have appeared like a more specific naming was better (the old "avoid premature refactoring" mantra). I expect the product thinking at the time was all around "a brexit notice for a no deal scenario". As developers, the onus was really on us to ask: would we ever have more than one brexit notice? Even if we did, it would still be possible to distinguish them i.e. the future risk of confusion was always low.
Oh well. Easy to say all this with hindsight!
huwd
approved these changes
Dec 30, 2020
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.
https://trello.com/c/kUqdYsCD/746-update-the-wording-of-the-whitehall-brexit-callout-box
This makes a couple of changes to make future updates to the
callout easier. Please see the commits for more details.