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

Angular 7 #216

Merged
merged 6 commits into from
Jan 11, 2019
Merged

Angular 7 #216

merged 6 commits into from
Jan 11, 2019

Conversation

hakimio
Copy link
Collaborator

@hakimio hakimio commented Jan 10, 2019

Recreated your project using latest version of Angular cli.

  • Now you have two projects: the library (projects/ng-chartist-lib) and the demo app (projects/ng-chartist-demo).
  • Unit tests still pass
  • Tslint tests also succeed
  • Demo still runs as expected
  • Added a couple more package.json scripts ("pack" and "package")
  • Tried to keep all the other scripts the same way as they were before
  • Couldn't get doc generator to work with Node 10, but should still work with older versions

Let me know if the changes look ok to you.

…and as a result:

* The library works correctly now with Angular 6 and 7.
* AOT builds work as expected.
@willsoto
Copy link
Owner

@hakimio At first glance this looks really promising. I will pull the branch locally tomorrow and just run through it real quick. Thanks so much for taking the time to do this.

@hakimio
Copy link
Collaborator Author

hakimio commented Jan 10, 2019

Cool. Just remember to build the library before building/running the demo app.

@hakimio
Copy link
Collaborator Author

hakimio commented Jan 11, 2019

It seems the issue with typedoc wasn't related to node version. Anyway, it's fixed now as well.

@hakimio
Copy link
Collaborator Author

hakimio commented Jan 11, 2019

Can you also fix Greenkeeper?

@willsoto willsoto merged commit 810ef8b into willsoto:master Jan 11, 2019
@willsoto
Copy link
Owner

@hakimio Thanks again. I'll take a look at greenkeeper. I'll try and cut a release as well

@hakimio
Copy link
Collaborator Author

hakimio commented Jan 11, 2019

@willsoto You might want to remove "yarn-error.log" from npm package.
Also, can you update the demo website?

@hakimio
Copy link
Collaborator Author

hakimio commented Jan 14, 2019

@willsoto since you removed the README file from "projects/ng-chartist" folder, ng-chartist page on npmjs.com says "Unable to find a readme for [email protected]".

Also, in your place I would keep 2 separate package.json files, since now the package.json file in the npm package contains lots of stuff which shouldn't be there.

@willsoto
Copy link
Owner

@hakimio Good catch on the README, I'll fix that right now. Regarding two separate package.json I am not a fan of that since it just complicates the release process slightly from my perspective. I've moved everything into devDependencies so it shouldn't affect consumers at all.

@hakimio
Copy link
Collaborator Author

hakimio commented Jan 14, 2019

@willsoto What do you think about using compodoc instead of typedoc for documentations? It has better structure, better styling and it's easier to configure it.

Do you mind if I update README formatting?
Why are all the methods public in the ChartistComponent class? Is that on purpose?
Should we make reference to chartist object an observable variable?
Any idea why greenkeeper doesn't work? Is it because of the old version of "greenkeeper-lockfile"?

@willsoto
Copy link
Owner

@hakimio

What do you think about using compodoc instead of typedoc for documentations?

I don't have a strong opinion either way I suppose, I'd be open to a PR

Do you mind if I update README formatting?

Anything in particular about the formatting? Just want more of an idea of what you mean

Why are all the methods public in the ChartistComponent class? Is that on purpose?

It was probably because I didn't know better when I wrote the library originally.

Any idea why greenkeeper doesn't work? Is it because of the old version of "greenkeeper-lockfile"?

Could be. I'll take a look at that soon.

@hakimio
Copy link
Collaborator Author

hakimio commented Jan 14, 2019

@willsoto
Basically, I want to make "Installation" and "Usage" sections in the readme look like in "cebor/angular-highcharts" library.

@willsoto
Copy link
Owner

Ok. I like that.

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.

2 participants