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

add distanceX and distanceY to tooltip #250

Merged
merged 5 commits into from Feb 3, 2017
Merged

Conversation

senluchen2015
Copy link
Contributor

@senluchen2015 senluchen2015 commented Jan 19, 2017

  • distanceX and distanceY, if given overrides the distance option.
  • change clampPosition to the edge and not opposite. ie. if you go distanceY = 400 it will max out on the top of the graph.

Demo:
http://jsfiddle.net/ucstdgsa/

@senluchen2015
Copy link
Contributor Author

@parikhshiv and @narenranjit

When you guys get the chance can you take a look at this and #251?

The only thing I am still debating about is distanceX, right now a positive number will send the tooltip left while a negative number will send the tooltip right. Not sure if that is intuitive, so might be best to switch it.

@mmrj
Copy link
Contributor

mmrj commented Jan 19, 2017

@senluchen2015 i haven't looked at the code or demo, so take with a grain of salt -- but i vote for distanceX positive values moving right / negative values moving left like a standard number line.

@parikhshiv
Copy link
Contributor

Code looks good to me, but I'd agree with Molly that positive values should move the tooltip right (and vice versa). Also, I'm not able to reposition the tooltip in the jsfiddle example, but I might be doing something wrong.

@senluchen2015
Copy link
Contributor Author

Flipped the direction so that positive number means towards right on distanceX. Positive distanceY is still going up.

Demo:
http://jsfiddle.net/ucstdgsa/1/

@@ -38,6 +40,8 @@
var plotTop = this.options.chart.plotTop;
var plotHeight = this.options.chart.plotHeight;
var distance = this.options.tooltip.distance;
var distanceX = this.options.tooltip.distanceX ? this.options.tooltip.distanceX : distance;
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this break if distanceX is explicitly set to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this will fix

var distanceX = this.options.tooltip.distanceX !== undefined ? this.options.tooltip.distanceX : distance;
var distanceY = this.options.tooltip.distanceY !== undefined ? this.options.tooltip.distanceY : distance;

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change the check to !=null (which'll catch both null and undefined)

Copy link
Contributor

Choose a reason for hiding this comment

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

you could also use !=undefined (could be more implicit?)

Copy link
Contributor

Choose a reason for hiding this comment

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

*as long as we never overwrite undefined (same goes for null though)

Copy link
Contributor

Choose a reason for hiding this comment

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

@parikhshiv,
At one time, you could overwrite undefined, but it hasn't been possible since ES5. Besides, the build process (uglify) replaces undefined with void 0.

@senluchen2015
Copy link
Contributor Author

Updated tooltip distance to take object or number

tooltip: {
    distance: {
          x: 100,
          y: 20,
    }
}

http://jsfiddle.net/ucstdgsa/2/

@narenranjit narenranjit merged commit a7dfb34 into master Feb 3, 2017
@wglasshusain wglasshusain deleted the tooltip-distance branch August 1, 2023 00:58
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.

None yet

5 participants