-
Notifications
You must be signed in to change notification settings - Fork 608
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
Setting only width
when using autosize: fit
should not use default height in compiled Vega
#5133
Comments
|
Unfortunately, I was playing with it a little bit. The problem is we still need to step the height to step * cardinality, but then Vega will try to fit axis into the frame with height = step * cardinality, which doesn't work. See this spec. Thus, 1. is not feasible. If you really want this, we need 2 in Vega. Please file an issue in Vega if you really want this. |
width
when using autosize: fit
should use default height in compiled Vegawidth
when using autosize: fit
should not use default height in compiled Vega
I'm wrong lol. There is fit-x/y in Vega: https://vega.github.io/vega/docs/specification/#autosize. I'd wait until #5138 lands before implementing this to avoid massive conflicts. |
width
when using autosize: fit
should not use default height in compiled Vega
Once that lands, you probably want to:
Thanks for volunteering to help. :) |
So if you just use autofit: fit, would be still have the width/height set by the config defaults, and then if you specify only fitting along one dimension, we only override for that particular dimension? |
width
when using autosize: fit
should not use default height in compiled Vega
That's a good question. We should consider how autofit should interact with the introduction of step-based width/height as well as Consider the example you post above, given the spec says Currently in #5138, I do an awkward thing where we use Also what if we get This issue is complicated. It's definitely worth enumerating how all the relevant properties can interact ( |
My current thinking is that we should respect whatever the user declares, and never cleverly downgrade Do you think we should have the API be |
@willium Thanks for following up. Here are my thoughts:
I agree that we should generally respect whatever the user declares. However, I disagree that we should intentionally show broken chart when users specify an invalid specification, which would only makes it feels more like a bug to users. There are many places that we drop invalid specification (with a warning explaining why something is dropped) and render something more reasonable as it's friendlier. So I think we should follow the same approach here. In this case, Thus, it would make sense to drop to fit-y part and throw warning.
For width=step with fit-x, it's also similarly problematic as we can't fit-x to a step. As "width=step" and "fit-x" are conflicting, we need to define their precedence. Given there is no reasonable fixed width to read from, it would make sense to drop "fit-x" and stick to the step value.
Autosize can be However, |
That makes sense. To make sure I have this right: the compiled fit should correspond to the minimal set of explicitly defined dimensions (argmin over the specified fit dimension).
|
Can you explain to me why we need Other than that, your list looks right to me. |
If we do go this way, we could just keep the language as is (only fit or no fit) and just change the compiler to use the proper fit in Vega (and not apply the config width/height) |
Yes. In #5133 (comment), you proposed to change VL, though, right? I am not yet convinced that we need to change the VL language. |
Yep. We would’ve had to change VL if we did what I suggested in #5133 (comment) |
When both x and y-axes are both quantitative, you can still choose to fit only one of them by using either fit-x or fit-y. I'm not sure when would that be useful, but fit-x and fit-y definitely offers more possibility. Also, if we throw warning when we reduce "fit" to either fit-x or fit-y, then it's important to provide "fit-x" and "fit-y" , which do not produce warnings. |
hmm, this complicates things. Are you suggesting that:
compiles to fit in Vega (and apply width/height config)? then, should
compile to fit in Vega (and apply height config)? (note that this is different than the cases described in this comment) |
Ahh, I see that if we use the size from the config, then having |
Yes. Good point that it differs a bit from #5133 (comment). But we can simplify a lot of logic by saying that
Then the logic is simply "drop the 'x' part of fit when Note that we drop step when if x is continuous (as it's invalid), so this logic should come after we drop step. The same logic can be applied for "height" and "y". I think this actually simplifies the logic as compared to the comment above. |
if we do this, wouldn't it be impossible to specify something like
without vega-lite adding in the default height as well? Or is that intentional? |
Of I understand @kanitw's comment correctly, he assumes that we add fit-x and fit-y. |
Vega-Lite already always add default width/height if not specified (always a number for continuous and number or step for discrete) as in:
Basically, the width/height always have to come from somewhere anyway. So this issue is about how to apply the width/height (to just the axis or fit to axis + padding for label/title), not about how we get width/height.
Yes, my proposed scheme works better with width and height. |
Currently, fit has precedence over step (step is dropped). However, I believe taking this approach, with reverse this precedence:
Is this alright? An alternative to maintain some precedence over step:
|
Yes, we should change the old precedence, which is simply a leftover from the old code (before As discussed above:
To expand, if you drop explicitly specified step, you still need to read the width from somewhere to fit to the width. But the default discrete width is usually a step too, so you don't have a fixed width to read from. Thus, it's better to drop x part of "fit" when there is a step. For the cases, you describe:
I think 1. make sense. For 2., I think fit |
In case 1, step has precedence, and in case 2, fit has precedence. Do you think that'd be confusing? |
Yes. (Sorry, I just realize I have a typo -- "fit" (not step) should be dropped. -- corrected above.) So "fit" should be dropped in both cases. Note that we currently drop step and use the width from continuousWidth even though the axis is discrete. This is obviously a bug. |
See the compiled Vega from this example. The
height
is set the defaultconfig.view.height
of200
, but I'd expect this chart to only "fit" the width (since that is all I specified).To solve this, I imagine there are two options:
height
orwidth
when using"autosize": "fit"
with one of the dimensions specified."autosize"
that specifies only to"fit"
along one dimension.The text was updated successfully, but these errors were encountered: