Remove custom reset file and fix CSS throughout #213

Merged
merged 1 commit into from Jan 11, 2017

Projects

None yet

6 participants

@carvil
Member
carvil commented Jan 11, 2017 edited

In this PR, we have:

  • deleted the custom _reset.scss file;
  • changed the layout to be the default core_layout;
  • fixed the styles throughout in order to keep it working as expected;
  • fixed an existing issue with breadcrumbs in tablet and mobile versions
    (since breadcrumbs weren't nested under a wrapper, they would not
    align properly - #210);
  • fixed a double blue line that was in between the main body and the
    footer.

There are wraith tests to verify this here:

https://carvil.github.io/collections-wraith-tests/shots/gallery.html

As you can see from the gallery above, there are differences between most pages, but those differences are the fixes mentioned above.

Trello:
https://trello.com/c/ECPy2bbv/318-remove-custom-reset-scss-file-from-collections-in-favour-of-the-one-already-in-static

The wraith configuration can be found here: https://github.com/carvil/collections-wraith-tests

Paired with @carolinegreen on this

@carvil carvil Remove custom reset file and fix CSS throughout
In this commit, we have:
- deleted the custom `_reset.scss` file;
- changed the layout to be the default `core_layout`;
- fixed the styles throughout in order to keep it working as expected;
- fixed an existing issue with breadcrumbs in tablet and mobile versions
  (since breadcrumbs weren't nested under a wrapper, they would not
  align properly);
- fixed a double blue line that was in between the main body and the
  footer.

There are wraith tests to verify this here:

https://carvil.github.io/collections-wraith-tests/shots/gallery.html
8178d68
@Davidslv
Contributor

Awesome work 👍

@tijmenb

👍 nice, well done both.

@tijmenb tijmenb merged commit 5e2bc76 into master Jan 11, 2017

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details
@tijmenb tijmenb deleted the remove-custom-reset-css-file branch Jan 11, 2017
- <main id="content" role="main" class="content <%= yield :page_class %>">
- <%= yield %>
- </main>
+ <div class="grid-row">
@fofr
fofr Jan 11, 2017 edited Member

I'm not sure the grid-row should be here.

Government frontend for reference:
https://github.com/alphagov/government-frontend/blob/master/app/views/layouts/application.html.erb#L22-L27

@@ -8,7 +8,12 @@
<% content_for :breadcrumbs do %>
<%= render partial: 'govuk_component/breadcrumbs', locals: @navigation_helpers.breadcrumbs %>
- <div class="header-context"><!-- Deliberately empty. This will prevent Static from adding a placeholder breadcrumb. --></div>
+ <div class="header-context">
+ <!--
@fofr
fofr Jan 11, 2017 Member

Could this use ERB comments to prevent the text rendering in the page?

@carvil
carvil Jan 11, 2017 Member

Yup, will change.

@fofr
Member
fofr commented Jan 11, 2017

Really happy to see this 💯
I've added some minor comments after merge, sorry.

@carvil
Member
carvil commented Jan 11, 2017 edited

@fofr we will address your comments in a new PR.
I thought grid-row would be the expected markup there. Should it be something else, or should we remove it entirely?

EDIT: Ok, from your example this should be deleted.

display: block;
- @include outer-block;
+ padding: 0 15px;
@@ -123,7 +123,7 @@
.browse-container {
background-color: $page-colour;
min-height: 500px;
- padding: 0 30px 45px 30px;
+ padding: 0 0 45px 0;
@nickcolley
nickcolley Jan 11, 2017

Nit: We should use long-hand where possible so it's clear what's happening here.

This is the equivalent of an a function with many arguments, whereas the long hand is easy to understand at a glance.

http://csswizardry.com/2016/12/css-shorthand-syntax-considered-an-anti-pattern/

@carolinegreen
carolinegreen Jan 11, 2017 edited

I'm not sure I agree with it being an anti-pattern. Even the post you've linked to it mentions things being less DRY as a caveat of using longhand. I think it is easier to look at first glance if it's shorter, and if this were a function, four arguments wouldn't necessarily be that many.

@fofr
fofr Jan 11, 2017 edited Member

I agree with @carolinegreen, as long as the zeroes for all those values need to be explicitly set I think it's fine. See also: alphagov/govuk-lint@dcdd0a9

@nickcolley
nickcolley Jan 11, 2017

I guess the way I see it is it's the difference between.

setPadding(0, 0, 45px, 0) and setPadding({ top: 0, left: 0, bottom: 45px, right: 0 })

padding: 0;
padding-bottom: 45px;

vs

padding: 0 0 45px 0;

If you can write the explicit long hand it makes it much easier to know what's happening without having to remember the ordering.

Not worth pushing for this PR though, wider discussion. 👍

@fofr
Member
fofr commented Jan 11, 2017

grid-row should be used when we want to divide a "row" of items, eg two-thirds one-third columns. It's a wrapper for grid classes.

Example in use:
https://www.gov.uk/government/case-studies/cash-transfers-help-for-those-who-need-it-most
screen shot 2017-01-11 at 13 44 54

@carvil carvil referenced this pull request Jan 11, 2017
Merged

Follow up css updates #215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment