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

Container spacing fixes for video release #5422

Merged
merged 18 commits into from
Aug 7, 2014
Merged

Container spacing fixes for video release #5422

merged 18 commits into from
Aug 7, 2014

Conversation

kaelig
Copy link
Contributor

@kaelig kaelig commented Aug 6, 2014

Right in time for release!

If someone from the content stream wants to ship it tonight, feel free to do so.

  • Prevent facia-container__inner from going full width in-between major breakpoints
  • Remove border on first container (content & fronts)
  • Add space between top border and titles in containers

Had to apply fixes in a few other places to cascade these styles when other components were mimicking them.

Basically, makes the video page look much better (see containers below the content itself):

image

And the network front:

image

Now for a standard article:

image

@@ -32,11 +30,14 @@
@mixin facia-container--layout-front {
.facia-container--layout-front {
.facia-container__inner {
@include mq(gs-span(7) + $gs-gutter*3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this off piste, or not defined as a tweakpoint inside _vars.scss?

@phamann
Copy link
Contributor

phamann commented Aug 6, 2014

👍 Thanks for doing this.

As you mention in the PR notes, I'm happy to ship this tonight, and you can resolve the comments in another PR tomorrow.

@phamann
Copy link
Contributor

phamann commented Aug 6, 2014

Seems like the build failure is a Sass linting error. I'll investigate and fix.

@phamann
Copy link
Contributor

phamann commented Aug 6, 2014

Both locally and on TeamCity i'm getting a Sass lint error on this branch, but it doesn't seem to be a normal failure (i.e. on a line number specifying the rule broken), instead the task just seems to be bombing out and throwing a not that helpful message.

>> scss-lint failed with error code: 1
>> and the following message:Error: Command failed:
>> 1 files are lint free

I'm suspicious that this may be related @kaelig's earlier of Sass and scss-lint today in #5393

It's too late to debug this evening, so am afraid I will wait until the morning.

@kaelig
Copy link
Contributor Author

kaelig commented Aug 6, 2014

Many thanks for the comments at such a late hour. I've fixed the scss-lint warnings and will update the node module tomorrow. Sorry about that.

Good point about the breakpoint naming in _facia-container.scss. I've started revisiting the way we name and declare breakpoints so that:

  • it reflects content (and not only articles)
  • it's up to date and easier to understand

It probably will take a medium-to-big PR to do that, but at least I won't be leaving dead bodies behind me when I go on holiday :)

You should be able to merge now.

phamann added a commit that referenced this pull request Aug 7, 2014
Container spacing fixes for video release
@phamann phamann merged commit b8212cb into master Aug 7, 2014
@phamann phamann deleted the container-spacing branch August 7, 2014 05:59
kaelig added a commit that referenced this pull request Aug 7, 2014
Container spacing pr fixes - follow up with #5422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants