-
Notifications
You must be signed in to change notification settings - Fork 17
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
Refactor Brexit callout #1958
Merged
Merged
Refactor Brexit callout #1958
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
20 changes: 10 additions & 10 deletions
20
...tylesheets/components/_header-notice.scss → ...tylesheets/components/_brexit-notice.scss
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,47 +1,47 @@ | ||
| .app-c-header-notice { | ||
| .app-c-brexit-notice { | ||
| margin-bottom: govuk-spacing(8); | ||
| } | ||
|
|
||
| .app-c-header-notice__header { | ||
| .app-c-brexit-notice__header { | ||
| background-color: govuk-colour("black"); | ||
| padding: govuk-spacing(4); | ||
| } | ||
|
|
||
| .app-c-header-notice__title { | ||
| .app-c-brexit-notice__title { | ||
| @extend %govuk-heading-m; | ||
| color: govuk-colour("white"); | ||
| margin: 0; | ||
| } | ||
|
|
||
| .app-c-header-notice__content { | ||
| .app-c-brexit-notice__content { | ||
| border-left: $govuk-border-width solid govuk-colour("black"); | ||
| border-right: $govuk-border-width solid govuk-colour("black"); | ||
| border-bottom: $govuk-border-width solid govuk-colour("black"); | ||
| padding: govuk-spacing(4); | ||
| } | ||
|
|
||
| .app-c-header-notice__text { | ||
| .app-c-brexit-notice__text { | ||
| margin-bottom: govuk-spacing(2); | ||
| } | ||
|
|
||
| .app-c-header-notice__links { | ||
| .app-c-brexit-notice__links { | ||
| margin-bottom: govuk-spacing(2); | ||
| } | ||
|
|
||
| .app-c-header-notice__list { | ||
| .app-c-brexit-notice__list { | ||
| margin-top: -5px; | ||
| margin-bottom: 0; | ||
| } | ||
|
|
||
| .app-c-header-notice__link-intro { | ||
| .app-c-brexit-notice__link-intro { | ||
| margin-bottom: govuk-spacing(2); | ||
| } | ||
|
|
||
| .app-c-header-notice__link { | ||
| .app-c-brexit-notice__link { | ||
| @extend %govuk-link; | ||
| font-weight: bold; | ||
| } | ||
|
|
||
| .app-c-header-notice__content .app-c-header-notice__text:last-child { | ||
| .app-c-brexit-notice__content .app-c-brexit-notice__text:last-child { | ||
| margin-bottom: 0; | ||
| } |
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
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
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
2 changes: 1 addition & 1 deletion
2
app/views/components/docs/header-notice.yml → app/views/components/docs/brexit-notice.yml
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
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
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
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
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
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
54 changes: 27 additions & 27 deletions
54
test/components/header_notice_test.rb → test/components/brexit_notice_test.rb
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,65 +1,65 @@ | ||
| require "component_test_helper" | ||
|
|
||
| class HeaderNoticeTest < ComponentTestCase | ||
| class BrexitNoticeTest < ComponentTestCase | ||
| def component_name | ||
| "header-notice" | ||
| "brexit-notice" | ||
| end | ||
|
|
||
| test "doesn't render a header notice when no params are given" do | ||
| test "doesn't render a brexit notice when no params are given" do | ||
| assert_empty render_component({}) | ||
| end | ||
|
|
||
| test "doesn't render a header notice when no title is given" do | ||
| test "doesn't render a brexit notice when no title is given" do | ||
| assert_empty render_component(links: [{ title: "test", href: "/test" }], description: "description") | ||
| end | ||
|
|
||
| test "doesn't render a header notice when no description is given" do | ||
| test "doesn't render a brexit notice when no description is given" do | ||
| assert_empty render_component(links: [{ title: "test", href: "/test" }], title: "title") | ||
| end | ||
|
|
||
| test "renders a header notice with text correctly" do | ||
| test "renders a brexit notice with text correctly" do | ||
| render_component(title: "This is an important notice", description: "This is some important information about the content.") | ||
|
|
||
| assert_select ".app-c-header-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-header-notice__content p", text: "This is some important information about the content." | ||
| assert_select ".app-c-brexit-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-brexit-notice__content p", text: "This is some important information about the content." | ||
| end | ||
|
|
||
| test "renders a header notice with text correctly with a multi-line description" do | ||
| test "renders a brexit notice with text correctly with a multi-line description" do | ||
| render_component(title: "This is an important notice", description: ["This is some important information about the content.", "It runs on to two lines."]) | ||
|
|
||
| assert_select ".app-c-header-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-header-notice__content p", text: "This is some important information about the content." | ||
| assert_select ".app-c-header-notice__content p", text: "It runs on to two lines." | ||
| assert_select ".app-c-brexit-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-brexit-notice__content p", text: "This is some important information about the content." | ||
| assert_select ".app-c-brexit-notice__content p", text: "It runs on to two lines." | ||
| end | ||
|
|
||
| test "renders a header notice with an aria label" do | ||
| test "renders a brexit notice with an aria label" do | ||
| render_component(title: "This is an important notice", description: "This is some important information about the content.") | ||
| assert_select "section[aria-label=notice]" | ||
| end | ||
|
|
||
| test "renders a header notice with no link" do | ||
| test "renders a brexit notice with no link" do | ||
| render_component(title: "This is an important notice", description: "This is a description", link_intro: "This should not be shown", links: []) | ||
|
|
||
| assert_select ".app-c-header-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-header-notice__content p", text: "This is a description" | ||
| assert_select ".app-c-header-notice__link-intro", false, "A link intro shouldn't be shown when no links are provided" | ||
| assert_select ".app-c-brexit-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-brexit-notice__content p", text: "This is a description" | ||
| assert_select ".app-c-brexit-notice__link-intro", false, "A link intro shouldn't be shown when no links are provided" | ||
| end | ||
|
|
||
| test "renders a header notice with one link" do | ||
| test "renders a brexit notice with one link" do | ||
| render_component(title: "This is an important notice", description: "This is a description", links: [{ title: "test", href: "/test" }]) | ||
|
|
||
| assert_select ".app-c-header-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-header-notice__content p", text: "This is a description" | ||
| assert_select ".app-c-header-notice__link[href='/test']", text: "test" | ||
| assert_select ".app-c-brexit-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-brexit-notice__content p", text: "This is a description" | ||
| assert_select ".app-c-brexit-notice__link[href='/test']", text: "test" | ||
| end | ||
|
|
||
| test "renders a header notice with multiple links" do | ||
| test "renders a brexit notice with multiple links" do | ||
| render_component(title: "This is an important notice", description: "This is a description", link_intro: "Look at these links: ", links: [{ title: "test", href: "/test" }, { title: "test2", href: "/test2" }]) | ||
|
|
||
| assert_select ".app-c-header-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-header-notice__content p", text: "This is a description" | ||
| assert_select ".app-c-header-notice__link-intro", text: "Look at these links:" | ||
| assert_select ".app-c-header-notice__list .app-c-header-notice__link[href='/test']", text: "test" | ||
| assert_select ".app-c-header-notice__list .app-c-header-notice__link[href='/test2']", text: "test2" | ||
| assert_select ".app-c-brexit-notice__title", text: "This is an important notice" | ||
| assert_select ".app-c-brexit-notice__content p", text: "This is a description" | ||
| assert_select ".app-c-brexit-notice__link-intro", text: "Look at these links:" | ||
| assert_select ".app-c-brexit-notice__list .app-c-brexit-notice__link[href='/test']", text: "test" | ||
| assert_select ".app-c-brexit-notice__list .app-c-brexit-notice__link[href='/test2']", text: "test2" | ||
| end | ||
| end |
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!