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

Fix variable name error on developer api documentation #5173

Merged
merged 1 commit into from Jan 27, 2018

Conversation

jonquach
Copy link
Contributor

@jonquach jonquach commented Jan 22, 2018

Small changes on the developer api.md documentation for the method .getElementAtEvent(e), in the code snippet to get the item that was clicked on.

I believe the variable called here should be item, not firstPoint.

See here

@jonquach jonquach changed the title Fix variable name error on developer api documentation for Fix variable name error on developer api documentation Jan 22, 2018
@jcopperfield
Copy link
Contributor

jcopperfield commented Jan 22, 2018

@jonquach I'm not sure your observation is correct. The function in core/core.controllers.js reads:

getElementAtEvent: function(e) {
    return Interaction.modes.single(this, e);
}

Interaction.modes.single returns the return elements.slice(0, 1);, i.e. the first element. Therefore firstPoint correctly specifies the returned element.

@jonquach
Copy link
Contributor Author

While I tried using it I got an error on this, see this codepen I made.

You can see in the console:

Uncaught ReferenceError: firstPoint is not defined

The object return in item is an Array of ChartElement, and inside it there is no key containing firstPoint.

Maybe I'm using it wrong, do you have any insight on this ?

@jcopperfield
Copy link
Contributor

@jonquach I think I explained myself wrong. IMO it would be better to rename the variable item to firstPoint or firstItem or even firstElement instead of the other way around, so that it is clear what is returned.

@jonquach
Copy link
Contributor Author

jonquach commented Jan 23, 2018

@jcopperfield Oh I see! Sorry I misunderstood. I agree, it seems more consistent, made the changes

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

5 participants