pattern-library / pattern-library-utilities

General utilities for the pattern library
MIT License
2 stars 2 forks source link

Add importer #10

Closed scottnath closed 9 years ago

scottnath commented 9 years ago

@sergesemashko @conortm

I have transferred over the relevant files from pattern-importer. I added tests for all the helper functions which are used during importing of files.

I have purposefully left ./lib/pattern-importer incomplete and with a TODO in it for using gulp to write out imports.

The main file to pay attention to is ./lib/get-pattern-import-data.js This file is what does the major work in parsing the pattern.yml file and making determinations for where files should be written.

scottnath commented 9 years ago

@sergesemashko @conortm

I have added the ability to have multiple script or style files.

creates an array to iterate over styles: https://github.com/scottnath/pattern-library-utilities/blob/add-importer/lib/get-pattern-styles.js#L33

creates an array to iterate over scripts: https://github.com/scottnath/pattern-library-utilities/blob/add-importer/lib/get-pattern-import-data.js#L114

tests added for new functionality: https://github.com/scottnath/pattern-library-utilities/blob/add-importer/test/main.js#L240 https://github.com/scottnath/pattern-library-utilities/blob/add-importer/test/main.js#L206

scottnath commented 9 years ago

@conortm @beynya @e2tha-e @sergesemashko

Do you have time to check out this pull request?

thanks, Scott

scottnath commented 9 years ago

Hi @conortm @beynya @e2tha-e @sergesemashko

This import system now has all pattern import code and instead of writing imported files on its own, it sends them to gulp.dest. The gulp task is include in this Pull Request.

Code is commented and unit-tested, please let me know if I can clarify anything here.

@conortm do you have anyone specific that could look over this pull request? It was first submitted two days ago. It will be difficult for me to move forward if this PR isn't reviewed.

e2tha-e commented 9 years ago

@scottnath cc: @conortm @beynya @sergesemashko

I had no idea of this repo's existence until checking my email a moment ago. I also see that generator-pattern-library is only in your personal GitHub account. Is this the plan going forward? Also, can those of us working on the Pattern Library be filled in on what to fork and install?

scottnath commented 9 years ago

Hi Ee,

The generator will definitely be moved into the Pattern Library github organization. Once it's there, it should be the upstream canonical.

To avoid further confusion I'll spend some time writing up some docs about what's where tomorrow.

Looking forward to having you jump on board

Scott

-------- Original Message -------- From: e2tha-e Sent: Thu, 10/09/2015 06:33 PM To: pattern-library/pattern-library-utilities CC: Nath, Scott (NBCUniversal) Subject: Re: [pattern-library-utilities] Add importer (#10)

@scottnathhttps://github.com/scottnath cc: @conortmhttps://github.com/conortm @beynyahttps://github.com/beynya @e2tha-ehttps://github.com/e2tha-e @sergesemashkohttps://github.com/sergesemashko

I had no idea of this repo's existence until checking my email a moment ago. I also see that generator-pattern-library is only in your personal GitHub account. Is this the plan going forward? Also, can those of us working on the Pattern Library be filled in on what to fork and install?

— Reply to this email directly or view it on GitHubhttps://github.com/pattern-library/pattern-library-utilities/pull/10#issuecomment-139401296.

scottnath commented 9 years ago

Hey @e2tha-e cc: @conortm @beynya @sergesemashko @rebmullin @gvorbeck @nikkiana

repos to fork

I have put together the list of the relevant repositories.

general information

I have a running page of information going on here: http://pattern-library.github.io/ not totally sure where else to put it at this point, so I'm just keeping that main org repo up-to-date as I have time.

generator location

scottnath commented 9 years ago

@e2tha-e

This pull request moves the Pattern Importing functionality out of the pattern-importer repository and into this repo. This will allow us to deprecate the pattern-importer repository.

Pattern Importing?

Each pattern library has it's own set of patterns in the ./patterns folder. A library optionally can import and use other pattern libaries via NPM.

The pattern importer reads through each pattern's data file (pattern.yml), and uses that info to transfer the pattern template and it's supporting files and data into Pattern Lab.

What's in this PR?

e2tha-e commented 9 years ago

@scottnath We should be adhering to the Drupal JS coding standards from here on out. I'm going to install JSCS and its Drupal plugin locally, but I think you should as well to preempt a line-by-line list of minor suggestions.

e2tha-e commented 9 years ago

@conortm @beynya @sergesemashko @rebmullin @gvorbeck @nikkiana

https://github.com/jscs-dev/node-jscs - a JavaScript coding standards validator. https://github.com/fluxsauce/jscs-drupal is the only plugin I found for Drupal. The .jscsrc looks pretty short compared to others so it might need some community contribution from us.

scottnath commented 9 years ago

Sounds great @e2tha-e

Installed it and cleaning the code up now

sergesemashko commented 9 years ago

@e2tha-e @scottnath, As far as I know Chillier will be using React.js and it most likely will fail Drupal JS Coding Standards. However there still will be Drupal components for admin part. The idea is good, I would suggest to postpone it till when we know where Drupal admin related JS is placed and apply Drupal js standards validator only for them.

e2tha-e commented 9 years ago

@sergesemashko @scottnath Does React.js have a set of coding standards. We can create a JSCS preset for that...

scottnath commented 9 years ago

@e2tha-e

Ran the system through jscs. Changes in latest commit. Please give specific nitpicks for anything I've missed.

sergesemashko commented 9 years ago

@e2tha-e, React.js has standards, but by adding rules for react.js we will drop out Drupal JS support... We kind of don't have any specific JS structure as we don't know yet all the requirements. It's hard to tell where will be placed what. Also, only eslint supports JSX linting, so jscs will not work well together with react.

e2tha-e commented 9 years ago

@sergesemashko @scottnath I don't have a problem with dropping Drupal JS standards so long as we stick to something. It seems like PubCentral will be less and less about Drupal "theming" on the frontend anyways. However, is there a chance we'll jump the React ship? With that in mind, I don't have a problem with using Drupal standards for everything that isn't React, and using React standards for everything that is. It's not ideal, but it's a level of complexity I can live with.

scottnath commented 9 years ago

@e2tha-e there is always a chance we'll jump any ship. We're in the middle of jumping ship right now from Publisher to PubCentral...obviously a timely change - but we should expect that the transitions in the future will be coming even more quickly now that we'll have a decoupled system.

See: "What happened to Web Components?" for a feeling of the future of web components and how they might work with react.

Also, react will not be the only code we develop using the pattern library. Our first project will be creating a drupal admin pattern library - and that work will be in twig templates.

We should definitely have required standards regardless of what type of code we're writing.

I added an .editorconfig and a .jscsrc to this repo for that purpose. A task in the current iteration, Typical Repository Files will help define what other files we should be adding for linting and standards. @conortm that task is currently assigned to you - should that become a whole story on it's own?

scottnath commented 9 years ago

@e2tha-e docs added to gulpfile.js

scottnath commented 9 years ago

morning @e2tha-e

I checked my unit tests, and I was only seeing one minor failing on the css test, which I fixed in 1560d26. I noticed yesterday your system showed a number of fails on the tests, I'm not seeing those here, fyi. Curious what the difference is in our systems...

scottnath commented 9 years ago

@e2tha-e @ericduran

all 'use strict' statements have been moved

scottnath commented 9 years ago

@e2tha-e getDefaultOptions in a gulp files now this:

var getDefaultOptions = function () {

scottnath commented 9 years ago

.jscsrc removed until https://rally1.rallydev.com/#/26707410139ud/detail/task/41507057658 is completed

scottnath commented 9 years ago

@e2tha-e afaik, I've covered all your current queries.

If you have time, please expand on your statement that the system needs a major overhaul. This is not meant as some bravado challenge - I honestly don't know what's wrong with what I've done or why it would be some huge undertaking to reverse it.

If you help me understand what you mean, I can improve both what I create and the systems I'm creating.

@ericduran the two files you requested major improvements on have been fixed and a ready for approval: https://github.com/scottnath/pattern-library-utilities/blob/add-importer/lib/get-category-path.js https://github.com/scottnath/pattern-library-utilities/blob/add-importer/lib/get-file-paths.js

scottnath commented 9 years ago

@e2tha-e

I have gone through every request in eslint and cleaned them up. There is one single issue in lib/convert-recursive-json-variables.js where eslint is wrong about variables, otherwise no errors at all.

Other than linting...are there any other outstanding issues needed to be checked to merge this PR?

thanks, Scott

e2tha-e commented 9 years ago

@scottnath I tested as much as I could and things look good at a unit level. Given the lack of feedback, I think it will be fine to effectively create private methods by writing functions outside of the exports properties. And I think it's a great step in the right direction to agree on a coding standard from here on out. Merging.

scottnath commented 9 years ago

thanks @e2tha-e !