Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Added clickannotation event #182

Merged
merged 6 commits into from
Nov 7, 2018
Merged

Conversation

doccray
Copy link
Contributor

@doccray doccray commented Apr 6, 2018

Here is a small patch which adds the clickannotation event.
The clickannotation event is already implemented in plotlyjs and an example is described here:
https://plot.ly/javascript/text-and-annotations/

@chriddyp
Copy link
Member

chriddyp commented Apr 9, 2018

Thanks @doccray , this looks good to me! A couple quick notes:

  • Do we need to clean this event data? i.e. are we passing in any extra data that we shouldn't be? (i.e. data keys that are prefixed by underscores like _fullData). Would you mind sharing a screenshot of e.g. a console.log(clickAnnotationData)?
    • I also want to make sure that it is serializable, as the other event data types are not (they have circular references). So, could you also share a JSON.stringify(clickAnnotationData, null, 2)?
  • Finally, could you update the CHANGELOG.md file and the version.py file?

Thanks!

@doccray
Copy link
Contributor Author

doccray commented Apr 12, 2018

Hi Chris,

here is the screenshot of console.log(clickAnnotationData):

image

Here is the output of JSON.stringify(clickAnnotationData, null, 2):
image

Is there any data like fullAnnotation which should be filtered?

Best regards,
Martin

@chriddyp
Copy link
Member

It seems like we should filter out fullAnnotation and event to be consistent with the other event data.

So, something like:

import {omit} from 'ramda'

const clickAnnotationData = omit(['event', 'fullAnnotation'], eventData)

If we add that, then I'm 👍 to merge.

@doccray - Let me know if you have time to make that change, otherwise one of us can step in and do it.

@doccray
Copy link
Contributor Author

doccray commented May 1, 2018

Hi Chris,
i have added the filtering of fullAnnotation and event. Thank you for the hint.
I have also updated the CHANGELOG.md file and the version.py file.
Best regards,
Martin

@valentijnnieman
Copy link
Contributor

@doccray Hi Martin. I'm going through our PR backlog and noticed this has never been merged. Sorry about that! I'm just going to go ahead and fix these merge conflicts so that we can merge it.

@valentijnnieman valentijnnieman merged commit 37544c8 into plotly:master Nov 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants