mobify / vellum

Default project styles for a mobile-first AdaptiveJS build.
MIT License
5 stars 0 forks source link

Prepare Vellum to work with Stencil #88

Closed ry5n closed 9 years ago

ry5n commented 9 years ago

For consistency Stencil components will all depend on a minimal set of shared variables. This PR ensures that Vellum plays nice with this set, and removes some unnecessary complexity from the existing variables file. Closes #72.

Status: Ready for review

Reviewers: @tedtate @jeffkamo @fractaltheory Ticket: RTM-128/CSOPS-1256 Linked PRs: compare with https://github.com/mobify/stencil-variables/blob/master/variables.scss

Changes

Made variable naming guidelines at the top of the file way more concise.

Removed or renamed:

Added:

fractaltheory commented 9 years ago

I don't know how much assurance I can provide as my SCSS knowledge and particularly, how we'll be using it in Stencils, is lacking. I think we need @jeffkamo on the case here to vet these changes, but you have my axe/:+1:

jeffkamo commented 9 years ago

Alright, these changes appear to check out well. I just have some questions above, but other than that this looking pretty good.

jeffkamo commented 9 years ago

@ry5n did you mean to have buttons and inputs to be the same size?

They appear slightly different at the moment.

screen shot 2015-05-28 at 3 17 22 pm

ry5n commented 9 years ago

Damn, buttons and inputs should be the same size. I’ll look into fixing that.

avelinet commented 9 years ago

Looks good to me @ry5n

ry5n commented 9 years ago

Thanks @avelinet !

jeffkamo commented 9 years ago

Looks like there's a merge conflict now. Fix that and you have my +1

ry5n commented 9 years ago

@jeffkamo Can you take a quick look again?

jeffkamo commented 9 years ago

Looks great! +1

ry5n commented 9 years ago

OK, will merge this closer to when Adaptive 2.0 ships.

ry5n commented 9 years ago

Closing to change merge target. See #90.