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

Fixes #4772: added scope for tooltip position mode call and added docs #4784

Merged
merged 4 commits into from Oct 10, 2017

Conversation

fsmeier
Copy link
Contributor

@fsmeier fsmeier commented Sep 21, 2017

See linked issue for more information #4772

@fsmeier
Copy link
Contributor Author

fsmeier commented Sep 21, 2017

@etimberg Is this ok? I think the biggest change here is the extended entry in the documentation :D

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Looks good, could be great to also include a unit test that checks expected arguments / scope.

@fsmeier
Copy link
Contributor Author

fsmeier commented Sep 22, 2017

@simonbrunel
Actually I tried to run gulp unittest, but it seems not to work inside my current setup docker-container. Any suggested container for this project?

22 09 2017 08:42:30.496:INFO [framework.browserify]: bundle built
22 09 2017 08:42:30.516:INFO [karma]: Karma v1.7.1 server started at http://0.0.0.0:9876/
22 09 2017 08:42:30.516:INFO [launcher]: Launching browsers Firefox, Chrome with unlimited concurrency
22 09 2017 08:42:30.526:INFO [launcher]: Starting browser Firefox
22 09 2017 08:42:30.536:INFO [launcher]: Starting browser Chrome
22 09 2017 08:42:30.536:ERROR [launcher]: No binary for Chrome browser on your platform.

@benmccann
Copy link
Contributor

I just did a quick google. It seems like the one below has node, chrome, and firefox. I haven't tested to see if it works with chart.js or not
https://hub.docker.com/r/atlassianlabs/docker-node-jdk-chrome-firefox/~/dockerfile/

@etimberg
Copy link
Member

Agree about the unit test. Once we that's in this is good to merge IMO

@fsmeier
Copy link
Contributor Author

fsmeier commented Oct 8, 2017

Yes! It is still on my todo. Planed it for tomorrow. The last two weeks were a bit stressful at work ;)

@benmccann
Copy link
Contributor

Thanks for adding the unit test!


expect(this instanceof Chart.Tooltip).toBe(true);
expect(elements instanceof Array).toBe(true);
expect(eventPosition.hasOwnProperty('x') && eventPosition.hasOwnProperty('y')).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't work because if the method is not called, then expectations are not evaluated. We could use spyOn instead to be sure that the method is actually called, something like that:

var scope = null;

Chart.Tooltip.positioners.test = function() {
    scope = this;
    return {x: 0, y: 0};
};

spyOn(Chart.Tooltip.positioners, 'test');

// {...}

node.dispatchEvent(evt);

var fn = Chart.Tooltip.positioners.test;
expect(fn.calls.count()).toBe(1);
expect(fn.calls.first().args[0] instanceof Array).toBe(true);
expect(fn.calls.first().args[1].hasOwnProperty('x')).toBe(true);
expect(fn.calls.first().args[1].hasOwnProperty('y')).toBe(true);
expect(scope instanceof Chart.Tooltip).toBe(true);

@fsmeier
Copy link
Contributor Author

fsmeier commented Oct 10, 2017

@simonbrunel I resolved your requested changes. Was a bit tricky...

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

@IMM0rtalis LGTM, thanks!

expect(fn.calls.first().object instanceof Chart.Tooltip).toBe(true);

I knew about this object property on the call object but I thought it was referring to the spied object (Chart.Tooltip.positioners), not thisArg. Nice!

@fsmeier
Copy link
Contributor Author

fsmeier commented Oct 10, 2017

Actually I had to look for alternatives to scope = this. Scope was undefined all the time so I was wondering if the spyOn is not storing the context somehow.

@etimberg etimberg merged commit c83b03f into chartjs:master Oct 10, 2017
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
…ded docs (chartjs#4784)

* added scope for tooltip position mode call and added docs

* added test for positioner

* removed named func for lint

* resolved pull-request comments
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
…ded docs (chartjs#4784)

* added scope for tooltip position mode call and added docs

* added test for positioner

* removed named func for lint

* resolved pull-request comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants