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

Allow label-transform to place labels out of a chart's boundary with padding: 'Infinity' or Infinity #3252

Merged
merged 4 commits into from Sep 21, 2021

Conversation

chanwutk
Copy link
Contributor

@chanwutk chanwutk commented Jun 27, 2021

Add an option to label-transform.
Allow padding: 'inf'.

@domoritz
Copy link
Member

domoritz commented Jun 28, 2021

Hmm, can someone not already use Infinity? I think we could support your new use case with the existing API of numbers.

@chanwutk
Copy link
Contributor Author

@domoritz Do you mean we should let users pass in the value Infinity instead?
I thought that JSON does not support Infinity like JavaScript.

@domoritz
Copy link
Member

Ahh, right. We should still support it for people who don't use Vega via json, I think.

@chanwutk
Copy link
Contributor Author

So, should we make the behavior for padding: Infinity to be the same as padding: 'inf'?

@domoritz
Copy link
Member

So, should we make the behavior for padding: Infinity to be the same as padding: 'inf'?

I would expect that to be the case.

@chanwutk
Copy link
Contributor Author

chanwutk commented Jul 2, 2021

Should the string value be 'inf' or 'infinity'?

@jheer
Copy link
Member

jheer commented Sep 20, 2021

The nice part about 'inf' is that it is fairly readable, though I do think 'Infinity' is clearer and more consistent with JS numbers. However, that also changes the type signature of the parameter... One alternative might include using null to indicate no bounds, but I think that might be confusing for a parameter named "padding" -- I'd associate that with having no padding (or at least a default value). Another option might be to use negative numbers to indicate Infinite padding -- that is not as clear as text but seems reasonable as a way of saying "no limits" and keeps the types tidier (unless we already support negative numbers as a way of forcing an inset margin). Any thoughts on this, @chanwutk or @domoritz? I'm OK moving ahead with the text string 'Infinity' unless someone strongly prefers another option.

I also agree with Dominik that numeric Infinity should be supported for specs that are defined in JS directly (not JSON text).

@chanwutk
Copy link
Contributor Author

Thank you for the comment.
We already support negative padding value (this will tighten the space allowed for labels to be placed).
I am also OK with the text string 'Infinity'.

@jheer
Copy link
Member

jheer commented Sep 20, 2021

OK, so let's go with 'Infinity' and then I think we can merge.

@chanwutk chanwutk self-assigned this Sep 20, 2021
@chanwutk chanwutk changed the title Allow label-transform to place labels out of a chart's boundary with padding: 'inf' Allow label-transform to place labels out of a chart's boundary with padding: 'Infinity' or Infinity Sep 20, 2021
@jheer
Copy link
Member

jheer commented Sep 20, 2021

I see that that the typings have been updated, but the metadata for Vega's JSON schema has not been updated:

{ name: 'padding', type: 'number', default: 0 },

The parsing for JSON schema types is here: https://github.com/vega/vega/blob/master/packages/vega-schema/src/transform.js#L44

Unfortunately, I don't think we support union types at the moment, only the "flat" types listed in the switch statement in the linked method in vega-schema above. This design was intentional, to avoid having overloaded parameter types. Unfortunately, I think that prevents the design decided upon here. Sorry I didn't realize/remember this sooner.

@chanwutk
Copy link
Contributor Author

I see.
How about if we omit padding, the padding is default to 0.
But, if we set padding to null, then the padding can be Infinity?

@jheer
Copy link
Member

jheer commented Sep 21, 2021

Yes, null would work. We just need to include null: true as part of the parameter metadata.

@jheer jheer merged commit b5c1db5 into master Sep 21, 2021
@jheer jheer deleted the ck/label-padding branch September 21, 2021 15:57
@jheer jheer mentioned this pull request Sep 21, 2021
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.

None yet

3 participants