-
Notifications
You must be signed in to change notification settings - Fork 402
Chore: Upgrade PL UIKit Build Tools #904
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
Chore: Upgrade PL UIKit Build Tools #904
Conversation
sghoweri
commented
Jul 22, 2018
- Swaps out compiling PL's Sass from using Ruby Sass to the much faster (and more easily supported) Gulp Sass (which uses node-sass under the hood)
- Adds Gulp Clean CSS to minify CSS assets generated
- Adds a few helpful NPM scripts to simplify process of setting up and building UIKit's assets
- Updates all UIkit dependencies to latest supported versions (prep work for some much larger and more exciting changes a-brewin 😄)
- Adding in a freshly compiled UIkit build with the latest changes folded in
…sily supported) Gulp Sass (node-sass under the hood)
…test versions; adding fresh compiled version of PL assets
…uild tasks are all in working order
|
@sghoweri this is great - will check it out soon! |
| ) | ||
| ) | ||
| .pipe( | ||
| cleanCSS({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could argue that removing whitespace from the output reduces comprehension. @bradfrost do you have thoughts on this? See the resulting diff: https://github.com/pattern-lab/patternlab-node/pull/904/files#diff-c44923a0878fc1c446276a7c21a7666d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source maps can definitely help with the readability of the bundled assets shipping in PL's UIKit (without taking a big performance hit in the process).
I'd personally recommend two things:
-
That we include a
--prodconfig flag that the build checks against when compiling so we can get the best of both worlds: easier to read code when doing dev work (esp. local dev work), can shave off about28.4KBof CSS when consumers are shipping / using PL's UI in production + enough flexibility to keep everyone happy -
We remove precompiled assets like the
distfolder from Git so PL users can more easily customize when consuming (I'm still in support of including thisdistfolder when publishing to NPM though out of convenience!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye. the inclusion was definitely born out of a need for direct npm install without having to invest in a lifecycle hook. I think I am more comfortable with how to do that now, so a prepublish step of some sort I think would be great
| "description": "Front-end assets and templates for the default Pattern Lab workshop view", | ||
| "main": "gulpfile.js", | ||
| "scripts": {}, | ||
| "scripts": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love this!
bmuenzenmeyer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work Salem. It's a good, incremental improvement to the codebase.
I really like the introduction of the npm scripts. I understand we rely on adding assets into dist/ which I dont want to relitigate right now.
This PR caused me to question the current CONTRIBUTING.md guidelines, which state to run gulp --copy-dist=../../../public
My guess is that was historic cruft we refactored out and forgot to remove, along with the actual gulp logic this flag entails.
I've added #913 to address this.
…uild tasks are all in working order
…-build-tools Chore: Upgrade PL UIKit Build Tools
