Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

[WIP] Implementation of issue #6's proposal #7

Merged
merged 1 commit into from Feb 15, 2017
Merged

Conversation

mAiNiNfEcTiOn
Copy link
Contributor

@mAiNiNfEcTiOn mAiNiNfEcTiOn commented Feb 2, 2017

What does this PR do:

Changes the way travix-ui-kit works. Currently in order to build the UI bundles we need to access the node_modules/travix-ui-kit/ and run the webpack command in it in order to generate the bundles.

Also the theming capability was dependent on if an ENV var was set (THEME_PATH)

So, according to #6 's proposal, I created this PR to implement to concept of a builder which can also be imported via require('travix-ui-kit/builder');

So, usage of the CLI:

# Without watcher
node_modules/.bin/travix-ui-kit -c <pathToStoreCSS> -j <pathToStoreJS> -t <pathToThemeYAML>

# With watcher
node_modules/.bin/travix-ui-kit -c <pathToStoreCSS> -j <pathToStoreJS> -t <pathToThemeYAML> -w

When not provided, the theme file falls back to the _default.yaml.

Honestly I think the watcher is a goodie for this project's development only. Don't see a case where you want a watcher to the UI Kit in an external project.

Where should the reviewer start:

  • Diffs
  • Clone the repo, do npm link, then try to use it on an existing or new code (by doing npm link travix-ui-kit on the folder you want to use this project on).

Unit and/or functional tests:

Still working on them (hence the WIP on the title)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c118eb6 on add-cli-tool into a6672e4 on master.

Copy link
Contributor

@iwwwi iwwwi left a comment

Choose a reason for hiding this comment

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

Please update the README.md file for the usage of the packages with the latest changes. I would expect to see the guide on how to use the package as cli and also if changes for the development process are made, those should be updated/added to the documentation.

@@ -1,22 +0,0 @@
const path = require('path');
Copy link
Contributor

Choose a reason for hiding this comment

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

pls keep in mind that this file is used in the build-theme and build-theme:watch scripts. These scripts should be updated accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed ma'am, will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iwwwi I've refactored the code a little bit so that if we do:

node_modules/.bin/travix-ui-kit -c ./myDestForCss -j ./myDestForJs -t ./myThemeYamlFile.yml -w

it will detect changes on the yaml file and re-bundles the themes.scss which then re-triggers the bundling of the ui-bundle.* files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I am changing the package.json to basically use the builder :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 63a91ff on add-cli-tool into a6672e4 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6f92214 on add-cli-tool into a6672e4 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6c32031 on add-cli-tool into a6672e4 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 068f27f on add-cli-tool into a6672e4 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b204b68 on add-cli-tool into a6672e4 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-53.8%) to 46.154% when pulling c6ca632 on add-cli-tool into a6672e4 on master.

@mAiNiNfEcTiOn
Copy link
Contributor Author

Working on the tests, bear with me on this one :D

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.6%) to 95.37% when pulling 0cee81a on add-cli-tool into a6672e4 on master.

@mAiNiNfEcTiOn
Copy link
Contributor Author

Almost...there.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9e5eae8 on add-cli-tool into a6672e4 on master.

Options:

-h, --help output usage information
-V, --version output the version number
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is V capital?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from commander: tj/commander.js#560

Nevertheless it is also more common to do -v to add verbosity on node CLI tools. Still, it is possible to provide an override of the help I think.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 77e69bb on add-cli-tool into 679e08b on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9177675 on add-cli-tool into 6928ff6 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7e49bf2 on add-cli-tool into 6928ff6 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7e49bf2 on add-cli-tool into 6928ff6 on master.

Refactors code to have a CLI (registering the binary in the package.json) and watcher embedded

Moves the saving of the theme file to the getStyles.js, renaming it to 'getStylesAndSaveTheme.js'

Updates the jest.config.json to be compliant with the latest Jest's config format

Updates the package.json to use the builder for building and watching purposes

Updates the README.md with the usage of the CLI

Updates the development section

Adds comments to the code and fixes the 'usage name' of a module

Changes the jest config to include non-covered scripts

Adds tests for the builder/* files and creates a new module to specifically handle the generation of the theme file

Improves the linting and jest's config to work properly

Adds wallaby.js (a config file for wallabyjs.com's tool)

Adds the files with a '100%' coverage on statements

Updates the package.json to run the babel command in parallel with the build:*/build process

Fixes the missing commands
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8270be8 on add-cli-tool into f6575a8 on master.

@fahad19
Copy link

fahad19 commented Feb 15, 2017

💖

@mAiNiNfEcTiOn mAiNiNfEcTiOn merged commit 7ae54ec into master Feb 15, 2017
@mAiNiNfEcTiOn mAiNiNfEcTiOn deleted the add-cli-tool branch February 15, 2017 10:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants