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 support for Latex in legends #3018

Merged
merged 6 commits into from
Sep 18, 2018
Merged

Conversation

jonmmease
Copy link
Contributor

@jonmmease jonmmease commented Sep 15, 2018

This PR introduces a few small fixes for MathJax Latex names in the legend. I know this has worked in the past, but it has not worked consistently in my testing. When it "doesn't work" the entire legend is not shown, and there are SVG console errors complaining that rect width/height cannot be set to NaN.

I believe this to be a concurrency issue that is addressed in 74328f3. The issue is that the call to drawTexts (line 123 in src/components/legend/draw.js) triggers an async rendering call for MathJax text, and then the call to computeLegendDimensions (new line 130) assumes the text has already been rendered.

Commit f23eb74 introduces no new logic, it just applies the indent that was omitted in 74328f3 to make the logical changes clearer.

Commit 1482077 applies the same horizontal offset to latex legend values as are applied to non-latex legend values (this was zero previously). And then I moved this to a more appropriate location and refactored to extract the constant in 222357c.

Commits acf5d7b and 6c27224 re-enable the Latex legend string in the mathjax mock. The mock now renders consistently as follows:

mathjax

@etpinard etpinard added bug something broken status: reviewable labels Sep 17, 2018
@etpinard
Copy link
Contributor

Looking good to my 👀

Thanks for splitting commits f23eb74 and 74328f3 making this PR very easy to review.

@etpinard
Copy link
Contributor

Does this fix #2303 or/and #966 ?

@jonmmease
Copy link
Contributor Author

The alignment issue in #2303 is fixed by

Commit 1482077 applies the same horizontal offset to latex legend values as are applied to non-latex legend values (this was zero previously). And then I moved this to a more appropriate location and refactored to extract the constant in 222357c.

I don't think the scrollbar issue in #966 is reproducible in the current master. I've never seen a scroll bar (with or without this fix), but I can confirm that the particular example in the codepen of #996 renders fine now and without a scroll bar. So I'd call it fixed 🙂

@alexcjohnson
Copy link
Collaborator

I don't think the scrollbar issue in #966 is reproducible in the current master.

#966 seems to have disappeared between 1.34 and 1.35 despite the fact that they both have the horizontal alignment problem. Likely #2426 fixed the scrollbar issue, but since MathJax was unusable in legends until this PR, I'll let you close #966 when this PR is merged.

@alexcjohnson
Copy link
Collaborator

Fantastic work @jonmmease - 💃

@etpinard
Copy link
Contributor

Awesome work @jonmmease

This one will be released today as part of 1.41.1.

@etpinard
Copy link
Contributor

I'm all set to release 1.41.1, so I'll go ahead and merge this PR. I hope @jonmmease won't mind.

Thanks again for this fix!

@etpinard etpinard merged commit 515953a into plotly:master Sep 18, 2018
@xhluca
Copy link

xhluca commented Sep 23, 2018

Amazing! I was waiting for this one to fix my regression app. Any idea when 1.41.1 will be bundled for plotly.py? Thanks!

@jonmmease
Copy link
Contributor Author

@xhlulu it will be in plotly.py 3.3 which should be out by the end of next week. If plotly.js releases a 1.41.2 next week we'll try to line up with that.

@xhluca
Copy link

xhluca commented Oct 28, 2018

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants