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.

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

HTML Publications clean-up #394

Merged
merged 4 commits into from Jul 11, 2017
Merged

HTML Publications clean-up #394

merged 4 commits into from Jul 11, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jun 28, 2017

Some refactoring following #384, changes that we didn't want to block the release of the feature.

  • Separate contents list styles and grid classes
  • Remove custom HTML contents tracking
  • Tweak alignment of HTML publication title and published date
  • Combine HTML contents and dashed list styles – creating three modifiers: dashed, nested and numbered (this highlights the need for a local style guide)

Apply fixes from HTML publications to all contents

A detailed guide with a long contents list:

Before:
screen shot 2017-06-28 at 11 15 06

After:
screen shot 2017-06-28 at 11 14 59

HTML publication content styles applied to all contents

Multi-line contents list items are easier to read and there's clearer separation between each contents list item.

New Old
HTML Publications All other contents
1.3 line height with 7.5px padding 1.5 line height with 5px padding

screen shot 2017-06-28 at 11 25 00

Heading tweaks

Current:
screen shot 2017-06-28 at 11 40 37

New:
screen shot 2017-06-28 at 11 40 43

CSV (and before update):
screen shot 2017-06-28 at 11 41 33

cc @nickcolley @andysellick

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-394 Jun 28, 2017 Inactive
@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Jul 3, 2017

This looks sensible 👍

fofr added 4 commits Jun 27, 2017
* Use grid class name conventions
* Make it clear why these classes differ from standard grid classes
* Reduce nesting/specificity of contents list CSS
* Use one place for common styles: line-height, link styles
* Create three modifiers: dashed, nested and numbered
* Contents lists now share a partial that’s already tracked
* Undoes #383 which was intended to be temporary
* Increase vertical spacing to more closely resemble CSV heading in
Whitehall
* Reduce spacing between title and published date for more natural
line-spacing.
@fofr
Copy link
Contributor Author

@fofr fofr commented Jul 3, 2017

Rebased following #395

@fofr fofr requested a review from andysellick Jul 3, 2017

&:before {
content: "";
position: relative;

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

Is position relative needed on this before element?

content: "";
position: relative;
float: left;
width: $gutter-half + 5;

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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?

}
}

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

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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?

This comment has been minimized.

@fofr

fofr Jul 4, 2017
Author Contributor

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

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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

@@ -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")

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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.

This comment has been minimized.

@fofr

fofr Jul 4, 2017
Author Contributor

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.


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

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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?)

This comment has been minimized.

@fofr

fofr Jul 4, 2017
Author Contributor

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.

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

Ah, excellent.

li {
margin-left: $gutter-half + 5;
padding-right: $gutter-half + 5;
list-style-type: disc;

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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.

This comment has been minimized.

@fofr

fofr Jul 4, 2017
Author Contributor

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

&.contents-list-nested {
.nested-contents-item-h2 {
margin-left: 0;
list-style-type: none;

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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

padding-right: $gutter-half + 5;
list-style-type: disc;

@supports (content: "") {

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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

This comment has been minimized.

@fofr

fofr Jul 4, 2017
Author Contributor

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.

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

.direction-rtl & {

This comment has been minimized.

@andysellick

andysellick Jul 4, 2017
Contributor

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.

This comment has been minimized.

@fofr

fofr Jul 4, 2017
Author Contributor

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

@fofr
Copy link
Contributor Author

@fofr fofr commented Jul 11, 2017

@andysellick Happy to approve this?

@fofr fofr merged commit 6349fb4 into master Jul 11, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
security/snyk No new vulnerabilities
Details
@fofr fofr deleted the tweak-html-pub-grid-classes branch Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.