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

Added an ability to use Highstock and Highmaps #9

Merged
merged 2 commits into from
Mar 6, 2015

Conversation

lolmaus
Copy link
Contributor

@lolmaus lolmaus commented Mar 5, 2015

Fixes #2.

I have removed addBowerPackageToProject('highcharts-release#~4.0.4') and app.import(app.bowerDirectory + '/highcharts-release/highcharts.js');.

It is up to user whether to import Highcharts, Highstock and/org Highmaps, which are distributed from separate repos. I have added instructions to the readme.

The addon no longer automatically imports the Highcharts Bower package, letting user import desired package manually
"devDependencies": {
"highcharts-release": "~4.0.4"
"qunit": "~1.17.1",
"highstock-release": "~2.1.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to leave this in as a dependency? Or should users explicitly add the package(s) they want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary for building the demo and running tests. It doesn't get into users' apps.

@ahmadsoe
Copy link
Owner

ahmadsoe commented Mar 5, 2015

@lolmaus can we make the installation process more simple? leave the charts as default, and if users want highstock or highmaps they just need to put some config on Brocfile or enviroment.js.

@lolmaus
Copy link
Contributor Author

lolmaus commented Mar 6, 2015

@ahmadsoe, we can have ember-highcharts update bower.json during installation.

But importing the Bower package is a problem. If we let ember-highcharts do it, there is no way for the user to import a different one.

To mitigate that we'll have to introduce some kind of configuration variable accessible from the Node side of Ember CLI. I am not familiar enough with Ember CLI to do that.

And i believe having a bower.json update without automatically importing the package is not consistent.


I'm not trying to press you, but if you don't accept this, i'm gonna publish my fork as a separate package. I will of course use a different package name and leave proper credit in the readme.

I'm gonna try introducing basic dynamic data and i'll do it in my fork if our ways part. (upd: i see this is already implemented).

I hate community splits, so i would prefer to work in your package. Can you please accept this PR, sacrificing some installation convenience? And then, when somebody figures out how to conditionally import a Bower package, they do another PR restoring the convenience?

@ahmadsoe
Copy link
Owner

ahmadsoe commented Mar 6, 2015

Who says I am not gonna merge this? This is a good start to including highstocks and highmaps components on this addon.

ahmadsoe added a commit that referenced this pull request Mar 6, 2015
Added an ability to use Highstock and Highmaps
@ahmadsoe ahmadsoe merged commit 8e72754 into ahmadsoe:master Mar 6, 2015
@ahmadsoe
Copy link
Owner

ahmadsoe commented Mar 6, 2015

Merged.. thanks @lolmaus

@lolmaus lolmaus mentioned this pull request Mar 11, 2015
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.

Highmaps
3 participants