Skip to content
This repository was archived by the owner on Dec 11, 2021. It is now read-only.

Conversation

MichaelArestad
Copy link
Contributor

To address #29.

  • Added basic file structure with a few example modules
  • Added Sass and Autoprefixer tasks to Gruntfile.js

"grunt": "0.4.2",
"grunt-autoprefixer": "^2.1.0",
"grunt-contrib-jshint": "0.10.0",
"grunt-contrib-sass": "^0.8.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

We previously decided, to avoid an annoying ruby dependency when its likely most people have no other need for ruby, to use libsass. So this would be just grunt-sass for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Fixing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added grunt-sass but forgot to remove grunt-contrib-sass

display: table; }
.clearfix:after {
clear: both; }
/*# sourceMappingURL=style.css.map */
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm i'm not sure about how to get it to add the blank link ill have to look into it. Even github view complains about the lack of blank line so i'm sure there must be a way to get it to add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just not include it as it's a compiled file.

@jzaefferer
Copy link
Contributor

We could just not include it as it's a compiled file.

I can't find discussion about this, but while dealing with sass files on an unrelated problem, I wanted to provide some input on that.

The project I'm working on uses sass files and checks in compiled CSS files in parallel. These are minified, so each time a scss file changes, there's a one-line diff on the compiled CSS file. This is fine for serial development on master, since anyone can pull and look at pages without first having to build something. This is painful when rebasing anything, since every commit that changes any scss file will produce a conflict on the CSS file.

Maybe there's a strategy to avoid those conflicts that I'm not aware of? Or is it better to force everyone to compile scss themselves?

]
},
src: [
"*.css"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing indent. Though for single items, you shouldn't need an array anyway, just pass the string directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Will fix.

@MichaelArestad
Copy link
Contributor Author

This is painful when rebasing anything, since every commit that changes any scss file will produce a conflict on the CSS file.

That's partially why I might prefer to just not include the compiled file. As we're eventually going to have a build system, there's not a huge need to have the compiled file at all.

@jzaefferer
Copy link
Contributor

I suppose there is no way around for contributors to compile scss to css, right? So might as well go with that.

Btw. I remember something about only using scss features that are in future CSS specs (or something like that). Does that apply to the few lines that are in this PR?

@arschmitz
Copy link
Contributor

@jzaefferer the discussion was brief and in #css-chassis i would say this is still open for discussion

Issues like this are part of why i would prefer to not see built files in the repo.

@sfrisk any thoughts here?

@arschmitz
Copy link
Contributor

@jzaefferer yeah actually having built files could add confusion as you might end up with beginner contributors submitting patches on the css files rather then the scss

@jzaefferer as far as future features that is what was discussed. Im not entirely sure on whats in the specs so far. If we are going to have a rule like this we should probably add this to the css style guide to make it clear whats ok not ok to use. @sfrisk thoughts? we will want this settled before we commit any scss i would think?

@sfrisk
Copy link
Contributor

sfrisk commented Jan 27, 2015

I'm leaning towards removing the .css build files and sticking to .scss. I definitely could see the issue with beginners submitting fixes to the css rather than the scss.

Maybe edit the readme as part of this PR (or a separate one) with a "Getting Started" section that explains how to get everything up and running?

@arschmitz
Copy link
Contributor

+1 for adding getting started to readme

@jzaefferer
Copy link
Contributor

That information should go into CONTRIBUTING.md, though there should be a link to it in the readme.

@arschmitz
Copy link
Contributor

@jzaefferer that sort of information is in the readme in all other repos though getting started is probably not the right section name as much as "building chassis"

https://github.com/jquery/jquery-ui#building-jquery-ui
https://github.com/jquery/jquery-mobile#buildcustomization
https://github.com/jquery/jquery#how-to-build-your-own-jquery

@jzaefferer
Copy link
Contributor

So were mostly written before contributing guidelines were a feature. They should be moved.

@arschmitz
Copy link
Contributor

This does not seem to keep with the spirit, of contributing guidelines, outlines in the blog post you link to. These should be a set of guidelines for contributors to follow when doing a patch. Basically contribute.jquery.org. These are setup instructions, exactly what is whats meant to be in a readme. http://en.wikipedia.org/wiki/README

@sfrisk
Copy link
Contributor

sfrisk commented Jan 27, 2015

How about for now we just have a "Getting Started / Building Chassis" on the Readme for now, so all the necessary information someone might want to know after arriving to the repo is there on the first page. Might help with attracting contributors if they can find out the state of things and how to get started right there on the home page.

On that note, @arschmitz, for your #34 PR, maybe add to the readme the how-to for getting perf testing up and running?

@MichaelArestad
Copy link
Contributor Author

I'll remove the build files and add info to the README. 

Quick note: will miss the meeting today.

@MichaelArestad
Copy link
Contributor Author

Added a Getting Started section to the README and removed the built Sass file

map: true,

// Target-specific options go here.
// browser-specific info: https://github.com/ai/autoprefixer#browsers
Copy link
Contributor

Choose a reason for hiding this comment

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

There comments seem more like documentation for autoprefixer then comments about what we are actually doing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll just remove the comments here.

@arschmitz
Copy link
Contributor

@sfrisk agreed. I was looking around at how other projects do this. I don't think we need to explain git. Maybe just add a link to https://help.github.com/articles/set-up-git/ for setup of third party things like git and NPM there are great resources we don't have to maintain. Looking at other projects that use preprocessors they add pretty much no documentation even on building. I think since barrier to contribution, is a big deal to us, we want some documentation here though.

Personally here i would stick to the specific dependencies they need to install ( not how just list them with links ), and then the commands to run to actually build a working version of chassis with brief explanation.

@MichaelArestad
Copy link
Contributor Author

Removed git references and simplified the Readme a bit.


### Building

Once you've cloned Chassis to your machine, run `npm install` and `grunt build` from the root Chassis directory.
Copy link

Choose a reason for hiding this comment

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

grunt build - suggest you to have an npm alias for this, like in core - https://github.com/jquery/jquery/blob/master/package.json#L54 that way you would never have grunt version conflicts.

@battaglr
Copy link

@MichaelArestad:

That's partially why I might prefer to just not include the compiled file. As we're eventually going to have a build system, there's not a huge need to have the compiled file at all.

👍

I have learned the hard way that including the compiled file will cause some headaches sooner than later.

@arschmitz arschmitz mentioned this pull request Mar 3, 2015
@arschmitz
Copy link
Contributor

Closing replaced by #42

@arschmitz arschmitz closed this Mar 3, 2015
arschmitz pushed a commit that referenced this pull request Mar 5, 2015
Also adds build instructions to readme and build/watch tasks for builds

Closes gh-42
Ref gh-33
@MichaelArestad MichaelArestad deleted the add/29 branch March 15, 2015 20:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants