Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Switch to gulp-include for theme JS #16

Merged
merged 1 commit into from
Sep 8, 2016
Merged

Switch to gulp-include for theme JS #16

merged 1 commit into from
Sep 8, 2016

Conversation

cshold
Copy link
Contributor

@cshold cshold commented Sep 7, 2016 •
edited
Loading

  • Remove browserify and associated packages
  • Add gulp-include
  • Remove var = theme in each layout file
  • Switch module.exports to theme.module_name (child of window.theme)
  • Move global JS (a11y, rte) init functions to global.js
  • Delete uri-helpers.js as it iss never used
  • Added self-invoking functions to product.js, customers-login.js and customers-addresses.js

Demo site - http://carson-dev-shop-2.myshopify.com/
Output JS - https://cdn.shopify.com/s/files/1/0780/8671/t/5/assets/theme.js?15778989300786398011

@Shopify/themes-fed

@cshold cshold added this to the 0.8.5 Release Candidate milestone Sep 7, 2016
@cshold cshold self-assigned this Sep 7, 2016
@bertiful
Copy link
Contributor

bertiful commented Sep 7, 2016

👍

macdonaldr93
macdonaldr93 reviewed Sep 7, 2016
View reviewed changes
package.json.ejs
@@ -31,6 +29,7 @@
"gulp-cssimport": "3.0.2",
"gulp-eslint": "2.0.0",
"gulp-ext-replace": "^0.3.0",
Copy link
Contributor

@macdonaldr93 macdonaldr93 Sep 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

remove ^ from gulp-ext-replace, we want exact versions. Must have crept in there on another PR.

@cshold cshold force-pushed the gulp-include branch 2 times, most recently from b7ab90c to cbce44f Compare September 7, 2016 14:37
@macdonaldr93
Copy link
Contributor

macdonaldr93 commented Sep 7, 2016 •
edited
Loading

Two rando points:

  1. config.js => roots.js should we do src/scripts/theme.{js,js.liquid} so that you can have either one?
  2. Do we want to uglify vendor.js? All that code is minified anyways... Why not optimize it even more?

@t-kelly
Copy link
Contributor

t-kelly commented Sep 7, 2016

Need to wrap template scripts in self-initialized functions again. Exposing things like var cache to the global namespace.

Otherwise 👍

@cshold
Copy link
Contributor Author

cshold commented Sep 7, 2016 •
edited
Loading

Actually let's table these notes to their own PR:

config.js => roots.js should we do src/scripts/theme.{js,js.liquid} so that you can have either one?
Do we want to uglify vendor.js? All that code is minified anyways... Why not optimize it even more?

Currently we only watch .js files so changing to theme.{js,js.liquid} might have consequences I can't see it.

@cshold cshold force-pushed the gulp-include branch 2 times, most recently from 50535e5 to ce05e7b Compare September 7, 2016 17:34
@cshold
Copy link
Contributor Author

cshold commented Sep 7, 2016 •
edited
Loading

Any more comments here? Opened tickets (.js and .liquid support and uglify vendor scripts) for Ryan's suggestions on our waffle board.

macdonaldr93
macdonaldr93 reviewed Sep 7, 2016
View reviewed changes
src/scripts/global/global.js
var a11y = require('./../slate/a11y.js');
var rte = require('./../slate/rte.js');
window.slate = window.slate || {};
// =require ../slate/*.js
Copy link
Contributor

@macdonaldr93 macdonaldr93 Sep 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

this might be a bit dangerous. I reran the build with this branch and uri-helpers.js didn't get removed from slate when I reran the yeoman generator. Due to the wildcard, that script got concatenated into globals and broke the js.

@macdonaldr93
Copy link
Contributor

One last comment, but otherwise looks good. 🎩 👍

@macdonaldr93 macdonaldr93 mentioned this pull request Sep 8, 2016
Closed
@cshold cshold merged commit 56fccb7 into master Sep 8, 2016
@cshold cshold deleted the gulp-include branch September 8, 2016 13:10
t-kelly pushed a commit that referenced this pull request Jul 31, 2017
switch to exact version (#16)
0d5de9d
t-kelly pushed a commit that referenced this pull request Jul 31, 2017
switch to exact version (#16)
b2d20db
t-kelly pushed a commit that referenced this pull request Jul 31, 2017
switch to exact version (#16)
3cd1ebb
@lock
Copy link

lock bot commented Oct 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@cshold cshold

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cshold @bertiful @macdonaldr93 @t-kelly