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

Needs Updated Dependencies #145

Closed
mattdodge opened this issue Dec 8, 2014 · 13 comments
Closed

Needs Updated Dependencies #145

mattdodge opened this issue Dec 8, 2014 · 13 comments
Labels

Comments

@mattdodge
Copy link

The homepage of this package lists only d3 as a dependency. However, every example seems to use jQuery. I see that Issue #62 seems to be complete, but I am unable to get it completely working with only d3. It looks as though an adapter (jQuery, MooTools, zepto) is needed.

One example:

new Epoch.Time.Line({el: '#graph', data: []})

If I try to declare a real-time line chart (without including width and height), I see failures due to this line:
https://github.com/fastly/epoch/blob/master/src/core/chart.coffee#L41
This is because d3 selectors do not have a width function like jQuery does. Doing this prevents the issue, but I would like to be able to use the auto width/height functionality.

new Epoch.Time.Line({el: '#graph', data: [], width: 300, height: 300})

This may just be a documentation issue, or it may be a bug, depending on the goal of the library. If the goal is to only have d3 as a dependency, there are some spots where the code needs fixing. If the goal is to have one of the provided adapters be a dependency, this probably needs to be reflected in the docs somehow

@rsandor
Copy link
Collaborator

rsandor commented Feb 6, 2015

Looks like you might be right, I am unsure how the unit tests missed this but they will need to be updated. @mattdodge - Would you mind perhaps digging in, fixing the tests, and the core implementation? It would be a great help if you could open a PR that addresses the issues you've found.

@rsandor
Copy link
Collaborator

rsandor commented Oct 4, 2015

No idea if this is still an issue. If anyone is seeing the same issue, please report it here.

@rsandor rsandor added the bug label Oct 4, 2015
@rsandor
Copy link
Collaborator

rsandor commented Oct 5, 2015

I am unable to reproduce this. Works fine without the width and height options. I will see if this gets reported again, but closing for now.

@rsandor rsandor closed this as completed Oct 5, 2015
@mattdodge
Copy link
Author

It's been a while since I've looked at this, but pulling down the latest does seem to prevent any JS errors from popping up. I see there are width and height functions added to the d3.selection prototype, but those functions also appear to have been there when the issue was filed? I'm not seeing any errors either when running off of commit 91558a1 though, which seems to be the commit I would have been using when filing the original issue.

I haven't been using the library, so don't know how much I can help. But for what it's worth, I am still unable to get a chart to appear using the d3 syntax. The examples on the docs are still all jQuery dependent.

When running this:

new Epoch.Time.Line({el: '#graph', data: data});

I just see an empty chart like so:
screenshot 2015-10-05 11 35 14

But the documented jQuery syntax:

$('#graph').epoch({type:'time.line', data: data});

makes the chart render properly (or at least I see something):
screenshot 2015-10-05 11 36 00

I'm fine with this being closed since it's been so long. But figured you might want to be aware of what I was seeing when using only d3 (no jQuery). Let me know if I can help — I can set up a fiddle or plunkr if needed.

@rsandor
Copy link
Collaborator

rsandor commented Oct 5, 2015

Thanks is for following up on this, I was able to get a d3 only example
working last night. I'll reopen this and link to a gist for you to try when
I get home tonight. Hopefully we can figure out what is going on here :)
On Mon, Oct 5, 2015 at 11:38 AM Matt Dodge notifications@github.com wrote:

It's been a while since I've looked at this, but pulling down the latest
does seem to prevent any JS errors from popping up. I see there are width
and height functions added to the d3.selection prototype, but those
functions also appear to have been there when the issue was filed? I'm not
seeing any errors either when running off of commit 91558a1
91558a1
though, which seems to be the commit I would have been using when filing
the original issue.

I haven't been using the library, so don't know how much I can help. But
for what it's worth, I am still unable to get a chart to appear using the
d3 syntax. The examples on the docs are still all jQuery dependent.

When running this:

new Epoch.Time.Line({el: '#graph', data: data});

I just see an empty chart like so:
[image: screenshot 2015-10-05 11 35 14]
https://cloud.githubusercontent.com/assets/797259/10289799/320c65fa-6b55-11e5-90d7-b4e731e34133.png

But the documented jQuery syntax:

$('#graph').epoch({type:'time.line', data: data});

makes the chart render properly (or at least I see something):
[image: screenshot 2015-10-05 11 36 00]
https://cloud.githubusercontent.com/assets/797259/10289820/5587d302-6b55-11e5-99aa-ce6c69e79bab.png

I'm fine with this being closed since it's been so long. But figured you
might want to be aware of what I was seeing when using only d3 (no jQuery).
Let me know if I can help — I can set up a fiddle or plunkr if needed.


Reply to this email directly or view it on GitHub
#145 (comment).

@mattdodge
Copy link
Author

Awesome, sounds good!

@rsandor
Copy link
Collaborator

rsandor commented Oct 5, 2015

Also sorry it took so long to address the issue, the repository was locked
in a private org for quite some time.
On Mon, Oct 5, 2015 at 12:26 PM Matt Dodge notifications@github.com wrote:

Awesome, sounds good!


Reply to this email directly or view it on GitHub
#145 (comment).

@rsandor
Copy link
Collaborator

rsandor commented Oct 6, 2015

@mattdodge try this example:

<html>
<head>
  <script src="https://cdnjs.cloudflare.com/ajax/libs/d3/3.5.6/d3.min.js"></script>
  <!-- <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/2.1.4/jquery.min.js"></script> -->
  <script src="dist/js/epoch.js"></script>
  <link rel="stylesheet" type="text/css" href="dist/css/epoch.css">
</head>
<body>

  <div class="epoch" id="event-bar-chart" style="width: 500px; height: 150px"></div>

  <script>
  var chart = new Epoch.Time.Line({
    el: '#event-bar-chart',
    axes: ['right', 'bottom'],
    tickFormats: {
      right: function (d) {
        return '$' + (d / 100000).toFixed(3);
      }
    },
    data: [
      // First series
      {
        label: "Series 1",
        values: [{time: 1370044800, y: 1000000}, {time: 1370044801, y: 1000000}]
      },
      // The second series
      {
        label: "Series 2",
        values: [{time: 1370044800, y: 780000}, {time: 1370044801, y: 98000}]
      }
    ]
  });
  chart.draw();
  </script>
</body>
</html>

This seems to render just fine for me.

@rsandor
Copy link
Collaborator

rsandor commented Oct 6, 2015

Oh I think I just spotted the issue, you have to call .draw explicitly for some reason right now when you use the classes directly. I am not sure why that it is though.

@mattdodge
Copy link
Author

Ah, yep, that did it! chart.draw() to the rescue.

@rsandor
Copy link
Collaborator

rsandor commented Oct 6, 2015

We should probably have both methods work the same way, can you do me a
favor and open a new ticket for this issue?
On Tue, Oct 6, 2015 at 11:43 AM Matt Dodge notifications@github.com wrote:

Ah, yep, that did it! chart.draw() to the rescue.


Reply to this email directly or view it on GitHub
#145 (comment).

@mattdodge
Copy link
Author

No problem, threw it up over at #195

@rsandor
Copy link
Collaborator

rsandor commented Oct 6, 2015

Thanks man! If you have time to look into it I would appreciate it (super
busy with work this week). First place to look would be the
src/adapters/jquery.coffee to see what is going on there. Probably would
have to update the base chart in src/common/charts.coffee to add the
automatic draw call.
On Tue, Oct 6, 2015 at 2:11 PM Matt Dodge notifications@github.com wrote:

No problem, threw it up over at #195
#195


Reply to this email directly or view it on GitHub
#145 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants