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

Updates base_chart.rb to insert "google.charts.load" instead of "google.load". Updates GanttChart class to Gantt. #102

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kyleoliveira

kyleoliveira commented Apr 9, 2016

GanttChart isn't working as specified given the current Google Charts API. This seems to get it working again.

@winston

This comment has been minimized.

Show comment
Hide comment
@winston

winston Apr 10, 2016

Owner

@kyleoliveira Thanks for the PR! The gantt chart fix is good. The google.charts.load fix probably needs a bit more work though. According to https://developers.google.com/chart/interactive/docs/basic_load_libs#frozen-versions, it seems like it's not only that google.load has changed, but the params to it has been updated too.

Would you like to take a crack at that? Otherwise, I can only look at it end of the week or the following week.

Thanks!

Owner

winston commented Apr 10, 2016

@kyleoliveira Thanks for the PR! The gantt chart fix is good. The google.charts.load fix probably needs a bit more work though. According to https://developers.google.com/chart/interactive/docs/basic_load_libs#frozen-versions, it seems like it's not only that google.load has changed, but the params to it has been updated too.

Would you like to take a crack at that? Otherwise, I can only look at it end of the week or the following week.

Thanks!

@kyleoliveira

This comment has been minimized.

Show comment
Hide comment
@kyleoliveira

kyleoliveira Apr 10, 2016

Ah, interesting. I don't know if I'm going to have to do that this week (doesn't look too bad, though), but if I can I'll update the PR (assuming you're not going to merge until the params are fixed) and put a comment in about it. Otherwise, go for it when you get a chance. I don't think a week or two will hold up anything on our end 😉.

Thanks for making this gem, by the way! So far it's looking like it's going to make things a lot easier for our project.

kyleoliveira commented Apr 10, 2016

Ah, interesting. I don't know if I'm going to have to do that this week (doesn't look too bad, though), but if I can I'll update the PR (assuming you're not going to merge until the params are fixed) and put a comment in about it. Otherwise, go for it when you get a chance. I don't think a week or two will hold up anything on our end 😉.

Thanks for making this gem, by the way! So far it's looking like it's going to make things a lot easier for our project.

@kyleoliveira

This comment has been minimized.

Show comment
Hide comment
@kyleoliveira

kyleoliveira Apr 10, 2016

Juuuuuust kidding - almost sorted it over breakfast. There is one issue though. According to https://developers.google.com/chart/interactive/docs/basic_load_libs#basic-library-loading , you can only call google.charts.load() once and you're supposed to append the list of all packages needed on the page instead of just the one that we're doing now every time the chart is inserted.

I think I'd need a bit more time to sort out the right way to do that. If I was going to try, it almost feels like maybe the library should provide a helper or something inserts the include for the library and the google.charts.load for all packages on the page. Presumably the user would pass in the package list unless there's an easy way to grab all chart object from the page already, in which case I suppose this would be even easier to automate.

How would you like to go from here? As-is, this works as advertised with the limitation that you can only have one chart per page.

kyleoliveira commented Apr 10, 2016

Juuuuuust kidding - almost sorted it over breakfast. There is one issue though. According to https://developers.google.com/chart/interactive/docs/basic_load_libs#basic-library-loading , you can only call google.charts.load() once and you're supposed to append the list of all packages needed on the page instead of just the one that we're doing now every time the chart is inserted.

I think I'd need a bit more time to sort out the right way to do that. If I was going to try, it almost feels like maybe the library should provide a helper or something inserts the include for the library and the google.charts.load for all packages on the page. Presumably the user would pass in the package list unless there's an easy way to grab all chart object from the page already, in which case I suppose this would be even easier to automate.

How would you like to go from here? As-is, this works as advertised with the limitation that you can only have one chart per page.

@kyleoliveira

This comment has been minimized.

Show comment
Hide comment
@kyleoliveira

kyleoliveira Aug 2, 2016

I finally got some more time to work on this. I've refactored the helper method so that you can either use it as before, or you can use separate methods to load the libraries and load the chart callback. In the latter case, you'll now be able to load all the libraries you need for a given page as specified on that API page I mentioned earlier.

It's not going to pass Travis yet because I haven't updated the tests accordingly. Is this how you'd like to go with this? If so I can take a whack at the tests (or you could write them or just merge it if you want).

kyleoliveira commented Aug 2, 2016

I finally got some more time to work on this. I've refactored the helper method so that you can either use it as before, or you can use separate methods to load the libraries and load the chart callback. In the latter case, you'll now be able to load all the libraries you need for a given page as specified on that API page I mentioned earlier.

It's not going to pass Travis yet because I haven't updated the tests accordingly. Is this how you'd like to go with this? If so I can take a whack at the tests (or you could write them or just merge it if you want).

@kyleoliveira

This comment has been minimized.

Show comment
Hide comment
@kyleoliveira

kyleoliveira Aug 18, 2016

I've updated the tests to handle the new changes. @winston what do you think?

kyleoliveira commented Aug 18, 2016

I've updated the tests to handle the new changes. @winston what do you think?

@flexloop

This comment has been minimized.

Show comment
Hide comment
@flexloop

flexloop Jan 13, 2017

Hi All-

I can't get the gantt to work even with the updates above in 35ed317.

Do either of you have a version that currently works?

I also don't seem to get the syntax correct to pull a specific commit from the repository by commit.

Thanks-

Michael

flexloop commented Jan 13, 2017

Hi All-

I can't get the gantt to work even with the updates above in 35ed317.

Do either of you have a version that currently works?

I also don't seem to get the syntax correct to pull a specific commit from the repository by commit.

Thanks-

Michael

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment