Initial implementation #1

umaar · 2017-12-05T15:29:53Z

Type: feature

The following has been addressed in the PR:

There is a related issue Command: Theme generation cli#168
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

This PR adds in a new command dojo create theme which the readme explains a bit more on.

Causes dojo/widgets#398

tomdye

some initial thoughts

tomdye · 2017-12-05T19:22:56Z

src/createThemeFile.ts

@@ -0,0 +1,48 @@
+import renderFiles from '@dojo/cli-create-app/renderFiles';


I'm not sure that we should be creating dependencies between different commands like this. Looks like you copied this approach from cli-create-widget. As this is now used in multiple places is it possible that renderFiles and the template functionality it uses should be moved elsewhere? A cli-util package or something? Worth raising an issue for.

True I did find it unusual to consume a repo in that manner, but yep I copied the approach so though it was an established pattern! Just two q's, did you mean raise an issue in this repository? I did that: #2

Also: is that something we should do now, before this PR is merged?

Matt replied super quickly! #2 (comment) and is saying we have a helper in the cli repo which can be used for that, so still wondering if you think we should do that before merging this pr

PR submitted! dojo/cli#180

tomdye · 2017-12-05T19:24:31Z

src/questions.ts

+	message: 'What Package to do you want to theme?'
+}];
+
+function getFileQuestions(packageName: string, files: string[], cssDataFileExtension: string): inquirer.Question[] {


you can export these one by one, ie export function getFileQuestions()...

Just wondering, do you think that's better? I kind of like

export { packageQuestions, getFileQuestions, askForPackageNames, askForDesiredFiles }

Because it's easy to scan for exports? But it's not a big thing, we can change it to export function getFileQuestions if desired 👍

tomdye · 2017-12-05T19:26:14Z

src/questions.ts

+const packageQuestions: inquirer.Question[] = [{
+	type: 'input',
+	name: 'package',
+	message: 'What Package to do you want to theme?'


can you change this to accept multiple package names please as per the POC?

tomdye · 2017-12-05T19:27:10Z

src/run.ts

+	const selectedWidgets = await askForDesiredFiles(fileQuestions);
+
+	const themedWidgets = selectedWidgets.map((selectedWidget: string): WidgetDataInterface => {
+		const [fileName] = basename(selectedWidget).split(cssDataFileExtension);


just a nit for code style, can you put spaces inside the array please? ie. const [ filename ] =

codecov · 2017-12-07T13:02:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@efab671). Click here to learn what that means.
The diff coverage is 95.29%.

@@            Coverage Diff            @@
##             master       #1   +/-   ##
=========================================
  Coverage          ?   95.29%           
=========================================
  Files             ?        6           
  Lines             ?       85           
  Branches          ?        3           
=========================================
  Hits              ?       81           
  Misses            ?        3           
  Partials          ?        1

Impacted Files	Coverage Δ
src/flattenArray.ts	`100% <100%> (ø)`
src/convertSelectorsToCSS.ts	`100% <100%> (ø)`
src/questions.ts	`100% <100%> (ø)`
src/run.ts	`100% <100%> (ø)`
src/main.ts	`75% <75%> (ø)`
src/createThemeFile.ts	`85.71% <85.71%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efab671...b324abd. Read the comment docs.

umaar · 2017-12-12T11:45:24Z

@tomdye do you think this one is ok now?

umaar · 2017-12-20T17:46:09Z

Let me know what you think! @matt-gadd @tomdye thank you :)

umaar · 2018-01-02T16:09:59Z

@tomdye @matt-gadd friendly reminder!

tomdye · 2018-01-02T16:18:32Z

src/convertSelectorsToCSS.ts

@@ -0,0 +1,9 @@
+function convertSelectorsToCSS(selectors: string[]): string {


as this is only used in one place does it really need to be in it's own file?

Oops! I had merged seconds before you left this comment ha! Me personally I enjoy keeping small isolated modules/files but that's just a preference, would you rather it not be in it's own file?

tomdye · 2018-01-02T16:19:37Z

src/createThemeFile.ts

+const pkgDir: any = require('pkg-dir');
+const packagePath = pkgDir.sync(__dirname);
+
+async function createThemeFile({


not sure this needs to be an async function as it currently does not return anything

tomdye · 2018-01-02T16:20:42Z

src/flattenArray.ts

@@ -0,0 +1,5 @@
+export default function flattenArray(list: any[]): any[] {


only used in one place, does not need to be in it's own file

Removed the file anyway 👍

tomdye · 2018-01-02T16:22:19Z

package.json

@@ -23,16 +23,34 @@
    "url": "https://github.com/dojo/cli-create-theme.git"
  },
  "scripts": {
-    "test": "grunt test"
+    "test": "grunt test",
+    "build": "./node_modules/.bin/grunt release --pre-release-tag=rc --dry-run --skip-checks --initial --force"


not sure we have this anywhere else.

Removed, thought it could be useful for us while developing though

tomdye · 2018-01-02T16:22:48Z

src/run.ts

@@ -0,0 +1,69 @@
+import { Helper } from '@dojo/interfaces/cli';
+import * as fs from 'fs-extra';


does this need to be fs-extra anymore?

Ah yeah the reason is I had some problems with mockery and the native fs module, I also saw we did it in another test which is where I got the idea

tomdye · 2018-01-02T16:24:22Z

src/run.ts

+			};
+		});
+
+		allWidgets.push(themedWidgets);


if you change this to allWidgets.push(...themedWidgets); you can remove flattenArray altogether

tomdye · 2018-01-02T16:26:08Z

src/run.ts

+		allWidgets.push(themedWidgets);
+	}
+
+	await createThemeFile({


i'm not sure this is working exactly as planned here as createThemeFile doesn't currently return a promise.

umaar added 8 commits December 5, 2017 12:07

Use the starting template for new repo + adds tests

5d9ddf6

Moudlarise files + update grunt

fc49be9

Readme

52048cc

Typo

959b65b

Correct install instruction

15a647b

Improve docs

f2a601c

Adds empty functional test

085d4e5

Needs to export something?

1050c1d

tomdye requested changes Dec 5, 2017

View reviewed changes

Support multiple packages

12633d5

umaar mentioned this pull request Dec 7, 2017

Create a cli-util repo which common CLI utilities #2

Closed

umaar added 2 commits December 7, 2017 13:51

Code style

ddbfc12

Use the create files method from the CLI helper

b324abd

umaar mentioned this pull request Dec 12, 2017

Extract themed css dojo/widgets#403

Merged

3 tasks

umaar requested a review from matt-gadd December 20, 2017 17:46

dylans modified the milestones: beta.5, rc.1 Dec 22, 2017

umaar mentioned this pull request Jan 2, 2018

Command: Theme generation dojo/cli#168

Closed

tomdye approved these changes Jan 2, 2018

View reviewed changes

umaar merged commit a111a8f into dojo:master Jan 2, 2018

umaar deleted the initial-implementation branch January 2, 2018 16:18

tomdye reviewed Jan 2, 2018

View reviewed changes

umaar mentioned this pull request Jan 2, 2018

Address code review feedback #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation #1

Initial implementation #1

umaar commented Dec 5, 2017 •

edited

tomdye left a comment

tomdye Dec 5, 2017

umaar Dec 7, 2017

umaar Dec 7, 2017

umaar Dec 8, 2017

tomdye Dec 5, 2017

umaar Dec 7, 2017

tomdye Dec 5, 2017

umaar Dec 7, 2017

tomdye Dec 5, 2017

umaar Dec 7, 2017

codecov bot commented Dec 7, 2017 •

edited

umaar commented Dec 12, 2017

umaar commented Dec 20, 2017

umaar commented Jan 2, 2018

tomdye Jan 2, 2018

umaar Jan 2, 2018

tomdye Jan 2, 2018

umaar Jan 8, 2018

tomdye Jan 2, 2018

umaar Jan 8, 2018

tomdye Jan 2, 2018

umaar Jan 8, 2018

tomdye Jan 2, 2018

umaar Jan 8, 2018

tomdye Jan 2, 2018

umaar Jan 8, 2018

tomdye Jan 2, 2018

umaar Jan 8, 2018

		@@ -0,0 +1,48 @@
		import renderFiles from '@dojo/cli-create-app/renderFiles';

		@@ -0,0 +1,9 @@
		function convertSelectorsToCSS(selectors: string[]): string {

		@@ -0,0 +1,5 @@
		export default function flattenArray(list: any[]): any[] {

		@@ -0,0 +1,69 @@
		import { Helper } from '@dojo/interfaces/cli';
		import * as fs from 'fs-extra';

Initial implementation #1

Initial implementation #1

Conversation

umaar commented Dec 5, 2017 • edited

tomdye left a 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

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

codecov bot commented Dec 7, 2017 • edited

Codecov Report

umaar commented Dec 12, 2017

umaar commented Dec 20, 2017

umaar commented Jan 2, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umaar commented Dec 5, 2017 •

edited

codecov bot commented Dec 7, 2017 •

edited