Skip to content

Conversation

muffinresearch
Copy link
Contributor

@muffinresearch muffinresearch commented May 5, 2016

Fixes mozilla/addons#9581
Fixes mozilla/addons#9583
Fixes mozilla/addons#9584

  • Adds a basic header outline and a placeholder for the video.
  • Fixes spec on addons (mainly width due to box-sizing) and a few other tweaks.
  • Fix background colour of page
  • Fix background to image (this will need ux looking at it because it looks to be too low contrast)

Future TODOs

  • We need more specific spec for the header fonts etc.
  • The video part should be split out into a separate component but we have an issue for dealing with that.
  • I've not got into handling responsive content just yet as we need a spec to know what we're aiming for and the desired behaviour.
  • RTL

I'll file separate issues for any of these bits that we don't already have issues for.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2666616 on muffinresearch:add-header into 1c726b7 on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4902527 on muffinresearch:add-header into 1c726b7 on mozilla:master.

border-radius: 5px;
display: block;
flex-grow: 0;
margin-left: 50px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to need an alternate rule for RTL. If you express this in flex it will work though. The spec defines a bunch of specific values for these sizes so I think it would be nice to set those in variables and then let flex lay it out correctly.

$blockWidth: 680px;

header {
  $videoWidth: 215px;
  $videoHeight: 120px;
  $spaceBetween: 50px;

  display: flex;
  justify-content: space-between;
  width: $blockWidth;

  .content {
    width: $blockWidth - $videoWidth - $spaceBetween;
  }

  .play-video {
    display: block;
    height: $videoHeight;
    width: $videoWidth;
  }
}

@mstriemer
Copy link
Contributor

r+wc

@muffinresearch
Copy link
Contributor Author

muffinresearch commented May 5, 2016

All the RTL stuff is going to come later. I know there's stuff here that will need changing but we can deal with it then, it won't be a problem to do a quick pass on it once we have a debug RTL locale.

@muffinresearch muffinresearch merged commit 52a52eb into mozilla:master May 5, 2016
@muffinresearch muffinresearch deleted the add-header branch May 5, 2016 17:22
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 76a3d91 on muffinresearch:add-header into 1c726b7 on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4d00d6d on muffinresearch:add-header into 1c726b7 on mozilla:master.

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.

Update theme block styles Update add-on block styles Discovery header
3 participants