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

Resolves #5185.

@plotly/plotly_js

@archmoj archmoj added this to the v1.57.0 milestone Oct 8, 2020
@nicolaskruchten
Copy link
Contributor

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
Contributor

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 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
Contributor

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
Collaborator

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
Contributor

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 commented Oct 14, 2020

💃 merging then...

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

Successfully merging this pull request may close these issues.

fullJSON access to computed values
3 participants