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

Kickoff v9 #192

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

Kickoff v9 #192

wants to merge 42 commits into from

Conversation

ashleynolan
Copy link
Contributor

In progress

so we can make it easier to add variants of each color
- `$z-index` now a sass map
- `$type` & `$breakpoint`'s keys now use t-shirt sizing to make it easier to remember them
- new `$font-family` sass map to allow easy selection of common font-stacks e.g. `ko-font(sans)`
- helper classes moved to `_helper-classes.scss`
- removed some rarely used items e.g. `cite, dfn, mark`
- removed `.h1, .alpha` classes for heading sizing
- update to use new functions/vars
`base` is for the default font-size
this convert font-based px values to rems at the build stage
- All font-based values that are written in `px` will be converted to rems at build stage by a PostCSS function.
- The `ko-rem` function was removed and the `ko-font-size` (was added) is only used to get the value from the `$type` map.
We now use https://github.com/modularscale/modularscale-sass, we are able to have better options when it comes to sizing type.
`$line-height-base` was removed because it is not needed anymore
@@ -177,17 +177,17 @@ code {
// p.typeSpecimen::after { content: "Font stack: #{ko-font(base)}"; }

.sg-typeSpecimen--body {
font-size: ko-rem(30);
font-size: 30px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More a question than a comment – what’s the reason we are using pixels over the font-size mixin in the styleguide?

Copy link
Member

Choose a reason for hiding this comment

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

@ashleynolan as I mentioned in 4ba354f, all font-based values that are written in px will be converted to rems at build stage by the PostCSS-pxtorem function.

The reason is that all conversion functions written in Sass are extremely fragile, particularly when there's an unknown type used as the argument. The pxtorem process is extremely reliable and the new dev experience is much more easy to understand - just write in px and we'll do the rest. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

also, those values are specific and not sized using the modular scale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok – that makes sense if they don’t need to hit the modular scale functions ⚡️

Copy link
Contributor Author

@ashleynolan ashleynolan left a comment

Choose a reason for hiding this comment

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

Looks good to me – only one small question on the styleguide.

@mrmartineau
Copy link
Member

I'm pretty happy with these changes, shall we merge in today?

@jpdriver
Copy link

@mrmartineau any chance we could look at moving away from eyeglass as part of V9?

@mrmartineau
Copy link
Member

@jpdriver perhaps, what would be the replacement?

@ashleynolan
Copy link
Contributor Author

@mrmartineau Think we need to update the grid as well as otherwise this’ll break the breakpoints in there.

@jpdriver What’s the issue with using eyeglass?

@jpdriver
Copy link

@ashleynolan eyeglass appears to be solely geared towards building with node-sass.

if you want to compile Kickoff's SCSS with other tools (in my case Webpack) the @import statements referencing other NPM modules throw resolution errors.

thus far, i've been solving this by replacing the original source with a webpack-friendly copy of any files that use eyeglass imports:

  // Overwrite SCSS that uses Eyeglass (hack to avoid incompatibility issues)
  var indexPath = 'kickoff/scss/kickoff.scss';
  var indexSrc = ownPath + '/template/src/' + indexPath;
  var indexDest = appPath + '/src/' + indexPath;
  spawn.sync('cp', [indexSrc, indexDest], { stdio: 'inherit' });

  var resetPath = 'kickoff/scss/_reset.scss';
  var resetSrc = ownPath + '/template/src/' + resetPath;
  var resetDest = appPath + '/src/' + resetPath;
  spawn.sync('cp', [resetSrc, resetDest], { stdio: 'inherit' });

that works ok, and it is only two files -- kickoff.scss and _reset.scss, but if we can find an alternate that works across environments that'd be neat...

@ashleynolan
Copy link
Contributor Author

@jpdriver We’re setting up a project using Webpack and Eyeglass and I don’t think we’ve had any issues so far with it. Have you raised it as an issue on the Eyeglass repo to try and get more info?

I haven’t tested out any decent alternatives, but the main thing I found that Eyeglass supported that other similar modules didn’t was resolving multiple packages pointing at the same dependency – namely not outputting the Sass for a module twice. Main use-case for this is if 2 modules happen to rely on another (for example, the grid) that it doesn’t create 2 lots of grid CSS.

@mrmartineau
Copy link
Member

@ashleynolan I actually started an issue on this subject and there appears to be some solutions.

What this effectively means is that we will move to use Webpack for compilation of both CSS and JS, thereby removing eyeglass, node-sass etc. This would mean that both the react projects and standard Kickoff projects would use the same build - this was always my intention.

@jpdriver
Copy link

i mean personally i'd like that even better; but is this going to fall into the same fate as V8 where we decided there were already enough moving parts without migrating the tooling too?

@mrmartineau
Copy link
Member

@jp if we can get it done, I would be massively happy.

@jpdriver
Copy link

ok how about we keep this as V9 without the tooling changes, but branch off this for V9 + only Webpack?

@ashleynolan
Copy link
Contributor Author

@mrmartineau Probably best to open a separate issue to discuss this, as seems this is a bigger discussion.

Wouldn’t a better solution be to modularise the gulp tasks – that way we could create separate modules that can be used dependent on the setup that someone wants?

I personally wouldn’t want the CSS builds to move to using Webpack – I like having the option of using Webpack or not and so baking it into the entire build would mean that Kickoff would effectively become useless to me. Tbh if we were to do that, we may as well get rid of Gulp then too and move the entire project over to Webpack.

@mrmartineau
Copy link
Member

@ashleynolan we could provide multiple build setups, but they all need to be maintained. Using Webpack to compile allows us to share build tooling with the react-based projects that we have. In my opinion, it shouldn't matter a given project uses to compile it's assets (one of the reasons we stuck with Grunt for so long), but we need to move with the times and allow an extensible setup that is more forgiving for the various project types that rely on the framework.

Regarding the removal of gulp: I don't think that we will remove it completely because gulp does some things that Webpack does not e.g. copy/delete tasks - which actually could be simple npm scripts, but I digress..

@mrmartineau
Copy link
Member

btw @jpdriver lets get a working version of a Webpack only setup (where possible) and compare.

@ashleynolan ashleynolan mentioned this pull request May 24, 2017
@ashleynolan
Copy link
Contributor Author

@mrmartineau @jpdriver Have opened an issue so can discuss that out of this PR.

@rentorm
Copy link
Contributor

rentorm commented Jun 1, 2017

I am also for full webpack support as I'm currently struggling to implement Kickoff in a project which is using only webpack and isomorphic-style-loader. @jpdriver have you used kickoff already in any project where only webpack is used for compiling JS and SCSS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants