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

Bowerizing #73

Closed
wants to merge 1 commit into from
Closed

Bowerizing #73

wants to merge 1 commit into from

Conversation

grayghostvisuals
Copy link
Contributor

Changes and adjustments based on Issue #66 which also includes aspects from #58.

A couple of notes:

  • Sass vars now have a !default flag allowing authors to override them from an outside source.
  • Theming "plain screen" involves another @extend placeholder (this can be improved upon as we go). The %modal-theme is your general/global defaults. Other separated placeholders can become more specific.
  • Grunt has been given some steroids and now includes MatchDep (So you don't have to add load task commands in your Gruntfile.js) and Live Reload because, hey, we like instant feedback right?
  • Eventually some mixins and functions should be added to manage the complexity/prefix grossness of the transitions and animations etc. Maybe adding auto prefixer would be better suited and a more maintainable option going forward.

Ok now lets discuss 🤘

@anselmh
Copy link
Contributor

anselmh commented Sep 29, 2013

Just had a quick look on it, here are my comments:

  • livereload is missing as dependency in package.json while it is called in Gruntfile.js
  • you have introduced a line-break issue at least in the package.json file (check the diff)
    In general I would prefer to have different smaller commits and Pull-Requests, one for each task. This makes it more maintainable and easier to merge parts of it.
    It would be great if you could fix the remaining issues first by amending the commit.

@drublic
Copy link
Owner

drublic commented Sep 29, 2013

Thanks so much for the commit! Separation LGTM at the moment but need to have a closer look.
Please stick to one task and one task only for each PR as Anselm said.

We haven't discussed including Grunt plugins other than we have. We don't need the tasks that you included, it's just something that is opinionated and I don't think we should add to much of this stuff in there. We could try to remove something tho…

Eventually some mixins and functions should be added to manage the complexity/prefix grossness of the transitions and animations etc. Maybe adding auto prefixer would be better suited and a more maintainable option going forward.

We don't want to add any complexity for developers. They should use it with Bower or just as a drop in, so there is no real space for prefixer or something. We had the sass-mixins repo as dependency in the early stage of this project and this didn't proof to be a good way to integrate with existing websites.

Again, thanks for the great work. We really appreciate it :)

@grayghostvisuals
Copy link
Contributor Author

@anselmh

livereload is missing as dependency in package.json while it is called in Gruntfile.js

Actually grunt-contrib-watch includes Live Reload already. https://github.com/gruntjs/grunt-contrib-watch#live-reloading

@grayghostvisuals
Copy link
Contributor Author

Please stick to one task and one task only for each PR as Anselm said.

I can remove the grunt options and just send back with the separated Sass (config, core and theme). LMK.

@drublic
Copy link
Owner

drublic commented Sep 29, 2013

Sounds perfect :)
In case you didn't know: You can just update this commit via git commit --amend and git push -f.

@grayghostvisuals
Copy link
Contributor Author

@drublic Thanks. G2K 🤘

@grayghostvisuals
Copy link
Contributor Author

@drublic Updated commit when you're ready. Thanks again for the tip above,

drublic added a commit that referenced this pull request Oct 7, 2013
Closes #58. Thanks to @fabien-d.
Closes #66.
Closes #73.
@drublic
Copy link
Owner

drublic commented Oct 7, 2013

Thanks for your work @grayghostvisuals. I've merged your changes into the wip-1.1.0 branch. Great stuff.

@drublic drublic closed this Oct 7, 2013
drublic added a commit that referenced this pull request Oct 7, 2013
Closes #58. Thanks to @fabien-d.
Closes #66.
Closes #73.
@grayghostvisuals
Copy link
Contributor Author

@drublic Thanks. I wish I could take the credit though. Most was from #58 (great work in there btw), but definitely happy to help out.

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.

None yet

3 participants