Skip to content

Conversation

@emmanuelle
Copy link
Contributor

I think there was a typo here, but I may be missing something.

@archmoj
Copy link
Contributor

archmoj commented Mar 6, 2020

Good catch.
@emmanuelle so because of this, hovertemplate was not working on histogram?

@emmanuelle
Copy link
Contributor Author

I'm very confused because hovertemplate does work as I just checked (screenshot from Python sorry but you'll get the idea)
image

@emmanuelle
Copy link
Contributor Author

of maybe the if was always True?

@archmoj
Copy link
Contributor

archmoj commented Mar 6, 2020

Most likely this should be labled as bug.
Anyway it would be good to know which issue we are closing.
@antoinerg any idea?

@antoinerg
Copy link
Contributor

Thank you @emmanuelle for spotting this ridiculous line of code of mine. Actually, it is completely unnecessary and it should be removed. All tests pass without it: d88d243

Please remove it and I'll approve the change 😸

@archmoj
Copy link
Contributor

archmoj commented Mar 9, 2020

Please wait Plotly.PlotSchema.get().traces.histogram.attributes.hovertemplate gives me

{valType: "string", dflt: "", editType: "none", arrayOk: true}

So in the case of blank string (i.e. the plotly.js default) hovertemplate is disabled.

@antoinerg
Copy link
Contributor

@archmoj I'm not sure I understand what you're getting at? Are you saying that d88d243 is not OK?

@archmoj
Copy link
Contributor

archmoj commented Mar 9, 2020

That commit is good; and we likely need to keep the if statement.
But wondering what should be the result in the case of a blank item in the array vs the blank default? Here is a demo.

@archmoj
Copy link
Contributor

archmoj commented Mar 10, 2020

TODO:

@archmoj
Copy link
Contributor

archmoj commented Mar 12, 2020

ping @emmanuelle

@archmoj
Copy link
Contributor

archmoj commented Mar 13, 2020

💃

@antoinerg antoinerg merged commit 9c7c398 into master Mar 13, 2020
@antoinerg antoinerg deleted the histogram-hovertemplate-typo branch March 13, 2020 20:29
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.

4 participants