Skip to content

Corrected typo in histogram hover #4618

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

Merged
merged 2 commits into from
Mar 13, 2020
Merged

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.

3 participants