The Wayback Machine - https://web.archive.org/web/20220519204140/https://github.com/plotly/plotly.js/pull/5203
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

Provide computed margins in full-json export #5203

Merged
merged 2 commits into from Oct 14, 2020
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 8, 2020

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Oct 8, 2020

I'd like this key to be called computed rather than _computed please, as leading underscores mean "private" in Python.

Also, we need an entry in the schema for this new key, so that the Python codegen adds the property accessors and so it appears in the reference documentation etc etc.

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Oct 9, 2020

Overall this looks good, but I'm seeing some odd behaviour with a figure like this one: https://codepen.io/nicolaskruchten/pen/wvWBmyQ

here the long x label is causing some automargin edge case: the right margin is increased because of the slanted label, but the bottom margin would exceed the cutoff so it resets. This is a known problem with automargins, and we don't need to tackle it now BUT the computed margin still shows r: 80 for me, which is incorrect in this case.

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Oct 9, 2020

Overall this looks good, but I'm seeing some odd behaviour with a figure like this one: https://codepen.io/nicolaskruchten/pen/wvWBmyQ

here the long x label is causing some automargin edge case: the right margin is increased because of the slanted label, but the bottom margin would exceed the cutoff so it resets. This is a known problem with automargins, and we don't need to tackle it now BUT the computed margin still shows r: 80 for me, which is incorrect in this case.

Here is a minimal codepen to illustrate the bug you mentioned.
@nicolaskruchten could you please confirm that it "sometimes" happening depending on the length of the long tick label?

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Oct 9, 2020

I haven't poked too much at when exactly it happens, sorry. Can we fix it in the case that I identified?

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Oct 14, 2020

Given that the issue already exists in fullLayout._size I'd be happy to consider that an independent bug and merge this PR as is, for its own part this PR looks great. But @nicolaskruchten I'd understand if you want to delay merging this until we solve that, since the bug isn't currently exposed to users.

@nicolaskruchten
Copy link
Member

@nicolaskruchten nicolaskruchten commented Oct 14, 2020

Let's merge this as-is for release in 1.57 for our sponsor, and then open a separate issue for the _size and prioritize fixing that one, as now some internal things are external ;)

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Oct 14, 2020

💃 merging then...

@archmoj archmoj merged commit 8128c88 into master Oct 14, 2020
9 checks passed
@archmoj archmoj deleted the export-computed-lauout branch Oct 14, 2020
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.

3 participants