Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addresses #375 allow foo.json #400

Merged
merged 1 commit into from
Jul 24, 2016
Merged

Conversation

stevemartin
Copy link
Contributor

Addresses #375

Summary of changes:

  • glob data files in data folder, excluding listitems
  • merge data as global data
  • tests inform code style

@geoffp
Copy link
Contributor

geoffp commented Jul 21, 2016

The code looks good to me!

@bmuenzenmeyer
Copy link
Member

I am crazy excited to see another thorough, well-thought out PR @stevemartin - you don't know how much it means to me and @geoffp to receive contributions! 😊

My only comment would again be dependencies. We have lodash and a custom merge function which I think should do the same thing as the merge library you included. Can you try either of those?

Again, this isn't meant to in any way degrade what you've done here. Merely trying to be mindful of dependencies/code bloat/redundancy as the project grows. THANK YOU!

* glob data files in data folder, excluding listitems
* merge data as global data
* tests inform code
* use lodash for merge
@stevemartin
Copy link
Contributor Author

@bmuenzenmeyer no problem, all good learnings! :)

@bmuenzenmeyer
Copy link
Member

will try to review this tomorrow!

@bmuenzenmeyer bmuenzenmeyer merged commit e76bc53 into pattern-lab:dev Jul 24, 2016
@bmuenzenmeyer bmuenzenmeyer mentioned this pull request Jul 26, 2016
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants