Skip to content
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.

Sign up for GitHub

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

Add callouts to Govspeak component; Remove information-block #899

Merged
merged 9 commits into from
Feb 10, 2017
Merged

Add callouts to Govspeak component; Remove information-block #899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
76ae1a7
Split information-block and call-to-action
36degrees Feb 10, 2017
6c6fe63
Apply rationalised design to call to action
36degrees Feb 10, 2017
77b0bac
Apply rationalised design to ‘example’
36degrees Feb 10, 2017
165a145
Add information callout
36degrees Feb 10, 2017
69bf6da
Add Govspeak warning callout
36degrees Feb 10, 2017
943d1e1
Remove ‘information block’ feature
36degrees Feb 10, 2017
e3f237c
Use a consistent margin for callouts
36degrees Feb 10, 2017
c2b7820
Fix linting errors
36degrees Feb 10, 2017
63cc97e
Remove information_block fixture
36degrees Feb 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/assets/stylesheets/govuk-component/_govspeak.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
@import "govspeak/attachment";
@import "govspeak/call-out-block";
@import "govspeak/call-to-action";
@import "govspeak/charts";
@import "govspeak/contact";
@import "govspeak/example";
@import "govspeak/footnotes";
@import "govspeak/fraction";
@import "govspeak/images";
@import "govspeak/information-callout";
@import "govspeak/legislative-list";
@import "govspeak/media-player";
@import "govspeak/stat-headline";
@import "govspeak/tables";
@import "govspeak/typography";
@import "govspeak/warning-callout";

.govuk-govspeak {
&.direction-rtl {
Expand Down
57 changes: 0 additions & 57 deletions app/assets/stylesheets/govuk-component/govspeak/_call-out-block.scss
View file
Open in desktop

This file was deleted.

25 changes: 25 additions & 0 deletions app/assets/stylesheets/govuk-component/govspeak/_call-to-action.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Govspeak call to action
// http://govuk-component-guide.herokuapp.com/components/govspeak/fixtures/call_to_action
//
// Support:
//
// - alphagov/whitehall: ✔︎
// - alphagov/govspeak: ✔︎

.govuk-govspeak {
.call-to-action {
margin: 2em 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do left and right margins need to be set to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not as that should be the default… this is a bit more defensive I guess. And I think I just prefer it to setting margin-top, margin-bottom and leaving the rest to chance?

background-color: $panel-colour;
padding: 2em;

&:first-child {
margin-top: 0;
}

p:last-child,
ul:last-child,
ol:last-child {
margin-bottom: 0;
}
}
}
13 changes: 10 additions & 3 deletions app/assets/stylesheets/govuk-component/govspeak/_example.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,16 @@

.govuk-govspeak {
.example {
border-left: 10px solid $panel-colour;
padding-left: 1.5em;
margin: 2.5em 0;
border-left: 1em solid $panel-colour;
padding: 1em 0 1em 1em;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👍 for using em in general but is this consistent with our spacing as defined here: https://govuk-elements.herokuapp.com/layout/?

spacing between elements is usually 5px, 10px, 15px or multiples of 15px

I'd prefer we redefine our spacing system to use em rather than be inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ @miaallers thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we redefine our spacing system to use em rather than be inconsistent.

Is this a recommendation for GOV.UK Publishing, or to all services?

Copy link
Contributor

@fofr fofr Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we redefine our spacing system to use em rather than be inconsistent.

That's a big change.

For now, using ems in this case improves legibility and makes spacing consistent with other content.

We're using the grid classes to define vertical margins on paragraphs etc though:
https://github.com/alphagov/static/blob/master/app/assets/stylesheets/govuk-component/govspeak/_typography.scss#L16-L17

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move to using multiples of $gutter-half (it’s essentially a difference of 15px vs a computed 16px).

It'd be good to better understand Mia's intentions. I'm aware Mia's out of the office today, so let's hold merging this till Monday, and discuss it then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were using those spacing rules but it seems like we're not so I'm 👍 to merge, we can have a conversation about consistency another time.

margin: 2em 0;

// Remove margin from the last element
p:last-child,
ul:last-child,
ol:last-child {
margin-bottom: 0;
}

strong {
display: block;
Expand Down
22 changes: 22 additions & 0 deletions app/assets/stylesheets/govuk-component/govspeak/_information-callout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Govspeak information callout
// http://govuk-component-guide.herokuapp.com/components/govspeak/fixtures/information_callout
//
// Support:
//
// - alphagov/whitehall: ✔︎
// - alphagov/govspeak: ✔︎

.govuk-govspeak {
.info-notice {
border-left: 1em solid $panel-colour;
padding: 1em 0 1em 1em;
margin: 2em 0;

// Remove margin from the last element
p:last-child,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this could be a mixin and shared between the callouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was debating it. I kinda feel it's in few enough places that it doesn't warrant hiding things behind abstractions, but I could be persuaded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be qualified? Would :last-child {} work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Probably not? It's hard to know exactly what elements are going to be used inside the callout and so what this could affect, but at the same time that's also a good argument for making it a generic :last-child…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@36degrees When I use comments to explain something it usually makes sense to refactor into a named function:

eg // Remove margin from the last element
becomes @include remove-last-element-margin;

I prefer the selector to be qualified. It's faster and less risky (eg otherwise it would apply to all of the last list items in a list).

If we use a mixin it's easy to add another case if found.

ul:last-child,
ol:last-child {
margin-bottom: 0;
}
}
}
41 changes: 41 additions & 0 deletions app/assets/stylesheets/govuk-component/govspeak/_warning-callout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Govspeak warning callout
// http://govuk-component-guide.herokuapp.com/components/govspeak/fixtures/warning_callout
//
// Support:
//
// - alphagov/whitehall: ✔︎
// - alphagov/govspeak: ✔︎

.govuk-govspeak {
.help-notice {
$icon-size: 34px;
$line-height-mobile: 20px;
$line-height-tablet: 25px;

margin: 2em 0;

// Add '!' icon
background-image: image-url("icon-important.png");
background-size: $icon-size $icon-size;
background-repeat: no-repeat;

@include device-pixel-ratio() {
background-image: image-url("icon-important-2x.png");
}

min-height: $icon-size;
padding-left: $icon-size;

// Center the icon around the baseline
padding-top: ($icon-size - $line-height-mobile) / 2;

@include media(tablet) {
padding-top: ($icon-size - $line-height-tablet) / 2;
}

p {
@include bold-19;
margin-left: 1em;
}
}
}
33 changes: 24 additions & 9 deletions app/views/govuk_component/docs/govspeak.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,6 @@ fixtures:
<p>My quote</p>
<p class="last-child">about things</p>
</blockquote>
call_out_blocks:
content: |
<div class="information-block">
<p>My important information</p>
<p>Information</p>
</div>
<div class="call-to-action">
<p>Call to action</p>
</div>
tables:
content: |
<table>
Expand Down Expand Up @@ -511,7 +502,31 @@ fixtures:
content: |
<div class="example">
<p>
<strong>Example</strong>
This is an indented example block.<br/>
It may span multiple lines, <a href="#">contain links</a>.
</p>
<p>
It may even span multiple paragraphs.
</p>
</div>
call_to_action:
content: |
<div class="call-to-action">
<p>Call to action</p>
</div>
information_callout:
content: |
<div class="application-notice info-notice">
<p>
If you drilled a tunnel straight through the Earth and jumped in, it would take
you exactly 42 minutes and 12 seconds to get to the other side.
</p>
</div>
warning_callout:
content: |
<div class="application-notice help-notice">
<p>
The water in the mouth of a blue whale weighs more than its body.
</p>
</div>