search

n3-charts / line-chart

Awesome charts for AngularJS.
http://n3-charts.github.io/line-chart/
MIT License
1.2k stars 179 forks source link

Adding the ability to define the radius of the dots Resolves #485 #494

Closed hambyiii closed 8 years ago

hambyiii commented 8 years ago

The dot radius for both the path and the tooltip can be set via the options object with the following format:

dots {
  pathRadius: 4,
  tooltipRadius: 6
}

If this is not set, it will default to 2 and 3 respectively

hambyiii commented 8 years ago

@lorem--ipsum The build is currently failing due to missing google-chrome-stable dependencies. I don't think that this could be related to my changes. Any advice?

lorem--ipsum commented 8 years ago

Okay so I've fixed the issue, please pull master and rebase from it then push again on your PR's branch and you should be all set

lorem--ipsum commented 8 years ago

Please rebase

hambyiii commented 8 years ago

Any chance of this getting a review? Would be awesome, since I am currently using a non-official version of the line-chart lib (one with my changes).

lorem--ipsum commented 8 years ago

Okay could you please add a e2e test and update the doc so we can see better what'd be the API now ?

Hint : to add an e2e tests, simply run gulp new-e2e-test --name=my_awesome_test.

hambyiii commented 8 years ago

sure thing

hambyiii commented 8 years ago

e2e tests and updated docs, as requested. :)

chaosmail commented 8 years ago

Just a quick question on this, why not use CSS to achieve this? Something like

circle.dot {
    r: 10px;
}

You can play around with an example here: http://codepen.io/chaosmail/pen/bpZBob In my opinion this setting would only make sense on a per series basis (in some use-cases), but just for styling all dots it's more convenient with CSS. What do you guys think?

chaosmail commented 8 years ago

What I like about the v2 is that it is fully customizable via CSS. So I think everything concerning styling that is not based on special series or data points (such as defining radius with a functon of the value) should be added as CSS and not as an option. But I would be happy to discuss this.

lorem--ipsum commented 8 years ago

Yeah I agree. I had this feeling yesterday without being able to exactly put my finger on it, but yeah, doing this via CSS makes more sense. How does that sound @hambyiii ?

hambyiii commented 8 years ago

Although it is possible to set the dot series via CSS, the same isn't true of the tooltip dot, since that is a path rather than a circle.

lorem--ipsum commented 8 years ago

Fair point. Then I think it would make more sense to change the tooltip dots to be circles. Originally I wanted them to be paths so they could have different shapes, but since it's never been done and it's in the way, feel free to change that.

Sorry for being picky, we really appreciate your interest in the project :-)

chaosmail commented 8 years ago

Yu can try my demo, it works also for tooltip dots (although a little differently)

hambyiii commented 8 years ago

Wrong dots :) I was referring to the actual dot that appears on the chart during mouseover, but as lorem--ipsum says, changing it to a circle and then doing everything via css might be the better solution

hambyiii commented 8 years ago

I'm still here, not gone away :) I am going to start a new branch for switching the tooltip dot from a path to a circle, if this is okay with you guys.

lorem--ipsum commented 8 years ago

That's great, thanks :)

hambyiii commented 8 years ago

Closing this pull request in favour of the new one I have created: https://github.com/n3-charts/line-chart/pull/505