Migrate task #108

cshold · 2017-02-01T21:08:28Z

This PR introduces slate migrate — a task to be used on an existing Shopify theme to get it ready for use with Slate tools.

Steps to test:

Download theme from Theme Store (or use slate build on existing theme and work from the dist directory)
Run slate migrate from root of theme

If not in the root migration will purposefully fail. We check for layout/theme.liquid before going too far

Run slate start

Note you will get an error because you don't have config.yml. Opened a bug to fix that here. The task works as it should otherwise.

@Shopify/themes-fed
cc @m-ux @macdonaldr93

macdonaldr93 · 2017-02-01T22:01:55Z

Tested on Windows 10, node 7.5 👍

NathanPJF

This review is just from reading the code. Haven't tried it out yet because I don't know how to make my globally installed slate use this branch. Could @cshold or @macdonaldr93 give me some instructions or point me to some docs?

NathanPJF · 2017-02-05T23:47:27Z

src/commands/migrate.js

+
+      if (!isShopifyTheme(workingDirectory)) {
+        console.log('');
+        console.error(yellow('  Your theme doesn\'t have /layout/theme.liquid. We have to assume your theme isn\'t a Shopify theme'));


Small change, but I prefer the message focus on the root directory instead of a "your theme isn't a theme" message.

Suggestion: ' The directory doesn't have /layout/theme.liquid. We have to assume this isn't a Shopify theme.'

NathanPJF · 2017-02-05T23:50:43Z

src/commands/migrate.js

+      const answers = await prompt({
+        type: 'confirm',
+        name: 'confirmation',
+        message: 'Warning! This will move files in your theme. Are you sure you want to proceed?',


Moving files in the theme sounds like it's going to be random. All this is doing is moving everything into a new src directory and creating two empty directories, correct?

Suggestion: 'Warning! This will change your theme's folder structure. Are you sure you want to proceed?'

macdonaldr93 · 2017-02-06T14:02:26Z

@NathanPJF sounds good 👍 I've updated the messages accordingly. To get this up and running, all need is slate to be symlinked (npm link) and then checkout this branch. Once you've checked out the branch, run npm start to build the dist folder.

NathanPJF · 2017-02-07T03:55:39Z

src/commands/migrate.js

+        return;
+      }
+
+      if (!existsSync(configYml)) {


While testing with no config.yml I realized this helpful message is really easy to miss as it's outputted at the top of a long list of console messages. Video: http://take.ms/fgZ3B

Let's put it as one of the last messages after a successful migration.

NathanPJF · 2017-02-07T03:58:15Z

README.md

+slate migrate
+```
+
+Converts an existing theme to work with Slate. Run this command from your project root to install dependencies and your theme files moved to to a `src/` directory. Use [theme commands](#theme-commands) to start developing. An empty `styles/` and `scripts/` folder will also be created.


Missing icons/ directory.

Updated instructions from other docs PR.

Converts an existing theme to work with Slate. Run this command from your project root to install dependencies and restructure your theme files into a `src/` directory. Empty `icons/`, `styles/` and `scripts/` folders will also be created. Create `config.yml` in your root using [this sample file](https://github.com/Shopify/slate/blob/master/config-sample.yml), then use [theme commands](#theme-commands) to start developing.

NathanPJF · 2017-02-07T04:01:08Z

src/commands/migrate.js

+
+      if (!isShopifyTheme(workingDirectory)) {
+        console.log('');
+        console.error(yellow('  The directory doesn\'t have /layout/theme.liquid. We have to assume this isn\'t a Shopify theme'));


Minor edit: "The current directory"

NathanPJF · 2017-02-07T04:28:30Z

src/commands/migrate.js

+      console.log('');
+
+      if (existsSync(srcDir)) {
+        console.error(yellow('  Your theme already has a src directory'));


I feel this isn't enough information to the user to quit the process.

Suggestion:

console.error(yellow(' Migrate task could not create a new src directory since your theme already has one.')); console.error(yellow(' Please remove or rename your current src directory.'));

NathanPJF · 2017-02-07T04:59:25Z

src/utils.js

+ */
+export function isShopifyThemeWhitelistedDir(directory) {
+  const whitelist = ['assets', 'layout', 'config', 'locales', 'sections', 'snippets', 'templates'];
+  return whitelist.indexOf(directory) > 0;


return whitelist.indexOf(directory) > -1;

This function is ignoring the assets/ directory since it is element 0. Video: http://take.ms/umfXJ

I totally had -1 and I don't know why I convinced myself 0 was right in a later commit lol Thanks for catching this.

macdonaldr93 · 2017-02-08T15:46:05Z

@NathanPJF ready for another round or approval

NathanPJF · 2017-02-13T21:31:31Z

@cshold @macdonaldr93 did another run through and it's good. I added an alias for migrate because it was the only one that didn't have one.

macdonaldr93 · 2017-02-14T15:02:37Z

Carson and I were chatting about the alias and Carson was thinking it would best not to have one. You wouldn't accidentally want to run this command. I'm indifferent. What do you think @NathanPJF ?

NathanPJF · 2017-02-14T15:50:45Z

@macdonaldr93 I'm indifferent. I can remove it.

Remove alias for `migrate`

macdonaldr93 · 2017-02-14T22:38:05Z

Made a new release (v0.9.0) and published 🎉 🎉

cshold and others added 3 commits February 1, 2017 13:05

Add documentation to migrate command

8670e73

Fix text

1f58a4d

Add migrate command

7f92d0b

cshold added the enhancement label Feb 1, 2017

cshold requested a review from NathanPJF February 1, 2017 21:08

cshold mentioned this pull request Feb 1, 2017

Add documentation for migrate command Shopify/slate#88

Merged

Ryan Macdonald added 2 commits February 1, 2017 16:15

Switch promises to async/await

67cf19c

Fix figures on Windows and colouring errors/warnings

83fcd94

Improve isShopifyThemeWhitelistedDir validation

112cdd4

macdonaldr93 force-pushed the migrate branch from 332b74a to 112cdd4 Compare February 1, 2017 22:08

NathanPJF suggested changes Feb 5, 2017

View reviewed changes

Update messaging based on feedback

dba5634

NathanPJF suggested changes Feb 7, 2017

View reviewed changes

Update based on feedback

bbab05b

add alias command

78f036a

NathanPJF approved these changes Feb 13, 2017

View reviewed changes

Update migrate.js

bbad9a6

Remove alias for `migrate`

macdonaldr93 merged commit f36b239 into master Feb 14, 2017

macdonaldr93 deleted the migrate branch February 14, 2017 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate task #108

Migrate task #108

cshold commented Feb 1, 2017

macdonaldr93 commented Feb 1, 2017

NathanPJF left a comment

NathanPJF Feb 5, 2017

NathanPJF Feb 5, 2017

macdonaldr93 commented Feb 6, 2017

NathanPJF Feb 7, 2017

NathanPJF Feb 7, 2017

NathanPJF Feb 7, 2017

NathanPJF Feb 7, 2017

NathanPJF Feb 7, 2017

macdonaldr93 Feb 8, 2017

macdonaldr93 commented Feb 8, 2017

NathanPJF commented Feb 13, 2017

macdonaldr93 commented Feb 14, 2017

NathanPJF commented Feb 14, 2017

macdonaldr93 commented Feb 14, 2017 •

edited

Loading

Migrate task #108

Migrate task #108

Conversation

cshold commented Feb 1, 2017

Steps to test:

macdonaldr93 commented Feb 1, 2017

NathanPJF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macdonaldr93 commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macdonaldr93 commented Feb 8, 2017

NathanPJF commented Feb 13, 2017

macdonaldr93 commented Feb 14, 2017

NathanPJF commented Feb 14, 2017

macdonaldr93 commented Feb 14, 2017 • edited Loading

macdonaldr93 commented Feb 14, 2017 •

edited

Loading