search

mobify / stencil

DEPRECIATED - The latest Stencil development is currently taking place in the Adaptive.js repo.
MIT License
4 stars 0 forks source link

Add vellum #106

Closed cole-sanderson closed 9 years ago

cole-sanderson commented 9 years ago

Status: Opened for Review Reviewers: @kpeatt @jeffkamo @ry5n @avelinet JIRA: ?

kpeatt commented 9 years ago

I think we should bower install Vellum for this. We don't need to register Vellum as a bower component to do that.

bower install mobify/vellum

cole-sanderson commented 9 years ago

@kpeatt Okay, I ll add vellum bower component.

ry5n commented 9 years ago

I want to clarify my position on this. I’m fine with adding Vellum for use in tests (a devDependency), but I feel uncomfortable with it as an end user dependency. I think that components should not depend on base styles to work correctly.

That said, many of our component styles already assume some of that, e.g. box-sizing: border-box so we are in an awkward in-between place. Personally I think components should declare all their necessary styles (they should be limited) and we should not make users import Vellum in order to use Stencil. Even for ourselves, it ensures that our components are self-contained enough to work independent of a project base stylesheet.

If we are going to add it here in either way (a dev dep or a user dep), we shouldn’t copy and paste as is currently done in the PR. It should be a bower dependency that gets imported somehow. If we are making it an end-user dependency then we start to run into the issue where each Stencil component conceptually /should/ @import vellum, which would cause it to be literally imported n times into the final output which is obviously bad. This last bit is definitely a separate issue but I have looked into it before and we could get around it with something like https://github.com/wilsonpage/sass-import-once.

jeffkamo commented 9 years ago

@ry5n I don't think we're going to make vellum an end user dependency. It's purely for testing, as in this the case of Stencil's visual tests.

We'll still generate vellum into the project and override as necessary, as we always have.

kpeatt commented 9 years ago

@ry5n The PR got updated to bring it in as a dev dependency.

cole-sanderson commented 9 years ago

Updated button component from vellum button width fix

kpeatt commented 9 years ago

@cole-sanderson Made some updates here but this is looking good. GtM from me!

cole-sanderson commented 9 years ago

@kpeatt looks good.

jeffkamo commented 9 years ago

:+1:

nastiatikk commented 9 years ago

NOTE: PR-110 dependent on this branch. Please don't remove it untill Tabs branch is merged to master or other PR is created.