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

Jump to bottom

HTML Publications clean-up #394

Merged
merged 4 commits into from
Jul 11, 2017
Merged

HTML Publications clean-up #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f657444
Separate contents and grid classes
fofr Jun 27, 2017
21cbaa2
Combine HTML contents and dashed list styles
fofr Jun 27, 2017
3ccf329
Remove custom HTML contents tracking
fofr Jun 28, 2017
ddd4f51
Tweak alignment of HTML pub title
fofr Jun 28, 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
33 changes: 0 additions & 33 deletions app/assets/javascripts/modules/track-html-publication-contents.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion app/assets/stylesheets/application.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// helpers for common page elements
@import 'helpers/available-languages';
@import 'helpers/back-to-content';
@import 'helpers/dash-list';
@import 'helpers/contents-list';
@import 'helpers/description';
@import 'helpers/sidebar-with-body';
@import 'helpers/share-buttons';
Expand Down
91 changes: 91 additions & 0 deletions app/assets/stylesheets/helpers/_contents-list.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
.contents-list {
margin-bottom: $gutter-two-thirds;

// Always render the contents list above a
// back to contents link
position: relative;
z-index: 1;
background: $white;
box-shadow: 0 20px 15px -10px $white;

li {
padding-top: $gutter / 4;
line-height: 1.3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Genuine question - should there be a unit of measurement for the line height or will all browsers safely default to em? (which is what I presume it's supposed to be?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://css-tricks.com/almanac/properties/l/line-height/#article-header-id-0

Unitless line heights are recommended due to the fact that child elements will inherit the raw number value, rather than the computed value. With this, child elements can compute their line heights based on their computed font size, rather than inheriting an arbitrary value from a parent that is more likely to need overriding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, excellent.

list-style-type: none;
}

// Remove underlines from lists of links
// to improve legibility
a {
text-decoration: none;

&:hover,
&:focus,
&:active {
text-decoration: underline;
}
}

&.contents-list-nested,
&.contents-list-dashed {
li {
margin-left: $gutter-half + 5;
padding-right: $gutter-half + 5;
list-style-type: disc;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line isn't needed - I don't believe there's any circumstances now where the list items should have a disc, it seems to always get overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used by browsers that do not support @supports (content: "—") {


@supports (content: "—") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a browser we support that doesn't support content: "-" ?

Copy link
Contributor Author

@fofr fofr Jul 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I remember having reason to keep the @supports when I switched away from the hack in April – however I haven't been clear in the commit message and I can't remember the reason why. 827a4af

This code isn't really being changed by this PR, just moved. Might be worth assessing as a separate issue.

list-style-type: none;

&:before {
content: "— ";
position: relative;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is position relative needed on this before element?

float: left;
width: $gutter-half + 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is width needed? If I turn it off in the inspector it doesn't appear to make any difference, I'd expect the width to be defined by the content?

margin-left: -($gutter-half + 5);
}
}
}
}

// When nesting contents lists, the top level items have no
// list marker and are styled in bold
&.contents-list-nested {
.nested-contents-item-h2 {
margin-left: 0;
list-style-type: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this needs to be defined here - the main styles for this list already set list-style-type to none.


&:before {
content: '';
}
}

.nested-contents-link-h2 {
font-weight: bold;
}
}

&.contents-list-numbered {
a {
display: table;
}

.contents-text,
.contents-number {
display: table-cell;
}

.contents-number {
min-width: 1.5em;
}

.contents-text {
$contents-text-padding: 0.3em;
padding-left: $contents-text-padding;

.direction-rtl & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been tested with proper RTL content? I don't have an example page so all I can do is apply the class to an English page, but if I do that the dashes in the contents list remain on the left hand side. Might be worth checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same code as was tested successfully in #395, just moved to a new file.

padding-left: 0;
padding-right: $contents-text-padding;
}
}
}
}
49 changes: 0 additions & 49 deletions app/assets/stylesheets/helpers/_dash-list.scss
View file
Open in desktop

This file was deleted.

58 changes: 5 additions & 53 deletions app/assets/stylesheets/views/_html-publication.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@
@include core-19;
color: $white;
background: $light-blue;
padding: $gutter;
padding: ($gutter * 2) $gutter $gutter;
margin-bottom: $gutter * 2;
}

// Based on the govuk-title component
.html-publication-title {
margin-bottom: $gutter;
margin-bottom: $gutter / 2;

.context {
@include core-27;
Expand All @@ -51,71 +51,23 @@
}
}

.contents-list-wrapper {
.column-quarter-desktop-only {
@include grid-column( 1 / 4, $full-width: desktop );

.direction-rtl & {
float: right;
direction: rtl;
text-align: start;
}

.contents-list {
position: relative;
z-index: 1;
padding-bottom: $gutter;
list-style: none;
background: $white;
box-shadow: 0 20px 15px -10px $white;

li {
padding-top: $gutter / 4;
line-height: 1.3;
}

.contents-text,
.contents-number {
display: table-cell;
}

.contents-number {
min-width: 1.5em;
}

.contents-text {
$contents-text-padding: 0.3em;
padding-left: $contents-text-padding;

.direction-rtl & {
padding-left: 0;
padding-right: $contents-text-padding;
}
}

a {
display: table;
text-decoration: none;

&:hover {
text-decoration: underline;
}
}

@include media(false, $desktop-breakpoint) {
box-shadow: none;
}
}
}

.contents-wrapper {
.column-three-quarters-desktop-only {
@include grid-column( 3 / 4, $full-width: desktop );
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to be collapsing to a single column at 640px, which I think is lower than HTML publications. It gets pretty compressed (particularly the contents list) just above that (in two column layout). Did you consider collapsing earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be the same as before (ie as added in #384), classes are just renamed.
Did you compare with live?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't, I'm just assessing it as it is.


.direction-rtl & {
float: right;
}
}

.offset-one-quarter {
.offset-quarter-desktop-only {
margin-left: 25%;

.direction-rtl & {
Expand Down
6 changes: 3 additions & 3 deletions app/views/content_items/html_publication.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
data-module="sticky-element-container"
>
<% if @content_item.contents.any? %>
<div class="contents-list-wrapper">
<%= render 'shared/sidebar_contents', contents: @content_item.contents, list_class: 'contents-list', format_numbers: true %>
<div class="column-quarter-desktop-only">
<%= render 'shared/sidebar_contents', contents: @content_item.contents, format_numbers: true %>
</div>
<% end %>

Expand All @@ -45,7 +45,7 @@
</div>
</div>

<div class="contents-wrapper <% unless @content_item.contents.any? %>offset-one-quarter<% end %>">
<div class="column-three-quarters-desktop-only <% unless @content_item.contents.any? %>offset-quarter-desktop-only<% end %>">
<%= render 'govuk_component/govspeak_html_publication', @content_item.govspeak_body %>
</div>

Expand Down
2 changes: 1 addition & 1 deletion app/views/content_items/specialist_document.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<div class="column-third">
<nav role="navigation">
<h2><%= t("content_item.contents") %></h2>
<ol class="dash-list" data-module="track-click">
<ol class="contents-list contents-list-nested" data-module="track-click">
<%= render 'shared/nested_content_item', headings: @content_item.nested_contents %>
</ol>
</nav>
Expand Down
3 changes: 1 addition & 2 deletions app/views/shared/_sidebar_contents.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
<%
list_class ||= "dash-list"
format_numbers ||= false
%>

<% if contents.any? %>
<nav role="navigation">
<h2><%= t("content_item.contents") %></h2>
<ol class="<%= list_class %>" data-module="track-click">
<ol class="contents-list contents-list-<% if format_numbers %>numbered<% else %>dashed<% end %>" data-module="track-click">
<% contents.each do |heading| %>
<li>
<% if format_numbers %>
Expand Down
37 changes: 0 additions & 37 deletions spec/javascripts/track-html-publication-contents.spec.js
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions test/integration/specialist_document_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ def test_meta(component)
test "renders a nested contents list" do
setup_and_visit_content_item('aaib-reports')

assert page.has_css?(".dash-list")
within ".dash-list" do
assert page.has_css?(".contents-list.contents-list-nested")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line necessary? Presumably test will fail anyway if not present, and this line isn't repeated at line 121 where another test does exactly the same thing. I guess the test might fail in a different way if the line is simply omitted, but if that's the case then it should also occur before line 121 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quite possibly, however this is a find/replace exercise to keep tests passing, rather than an assessment of every contents list test and its purpose.

within ".contents-list.contents-list-nested" do
@content_item['details']['headers'].each do |heading|
assert_nested_content_item(heading)
end
Expand All @@ -118,7 +118,7 @@ def test_meta(component)
test "renders a nested contents list with level 2 and 3 headings only" do
setup_and_visit_content_item('drug-device-alerts')

within ".dash-list" do
within ".contents-list.contents-list-nested" do
@content_item['details']['headers'].each do |heading|
assert_nested_content_item(heading)
end
Expand Down
2 changes: 1 addition & 1 deletion test/integration/topical_event_about_page_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class TopicalEventAboutPageTest < ActionDispatch::IntegrationTest

test "slim topical event about pages have no contents" do
setup_and_visit_content_item('slim')
refute page.has_css?('.dash-list')
refute page.has_css?('.contents-list.contents-list-dashed')
end

private
Expand Down
4 changes: 2 additions & 2 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ def assert_has_component_government_navigation_active(active)
end

def assert_has_contents_list(contents)
assert page.has_css?(".dash-list"), "Failed to find an element with a class of dash-list"
within ".dash-list" do
assert page.has_css?(".contents-list"), "Failed to find an element with a class of contents-list"
within ".contents-list" do
contents.each do |heading|
selector = "a[href=\"##{heading[:id]}\"]"
text = heading.fetch(:text)
Expand Down