-
-
Notifications
You must be signed in to change notification settings - Fork 668
feat: Support background to text marks #9646
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks. I think the spirit of this pull request is great. I wonder whether it would make more sense to support this in Vega? |
|
The idea to support this in Vega has been discussed before, for instance in this issue and here. In both cases @jheer suggested a solution in VL. Maybe his opinion has changed?
|
|
Ah, thanks for digging up those comments. Okay, let's see how we can tackle this in Vega-Lite then. |
|
I think it would be great to have easy access to this in vega-lite! I have needed this functionality often and it would be great to have a more automatic approach. A couple of thoughts:
Thanks! |
|
@kjgoodrick Thanks for your suggestions!
|
|
|
Alright, I’ve done some work on this and I think it’s ready for review:
Please let me know if I missed something. |
we have
Currently we don't have strong types for mark specific properties. I think it's fine to not address this in this PR. |
src/mark.ts
Outdated
| * | ||
| * __Default value:__ `2` | ||
| */ | ||
| bgPadding?: Padding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using padding for both rect padding and text strokeWidth is mixing up two different things a bit.
I think it's better to separate background from outline since they can have different sets of properties.
Maybe:
background: Color | ES | TextBackground,
outline: Color | ES | TextOutlinewhere
interface TextBackground{
color: ... // required
opacity?: ...
padding?: ...
}
interface TextOutline {
color: ... // required
strokeWidth: ...
...
// no padding, cornerRadius
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this feedback. I have separated the config for background and outline as you suggested, which has the added benefit of allowing a combination of both. I also have rewritten the example, tests and documentation. Please let me know if anything else is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kanitw If you have some time to spare, can you have a look at these changes and let me know if further work is needed? Thanks in advance.
PR Description
This PR introduces background support for text marks in Vega-Lite, improving readability and enabling basic annotation use cases.
Motivation
There has been interest in supporting backgrounds for text marks in the past (e.g., #1820, #3418). These discussions suggested that Vega-Lite could provide syntactic sugar to leverage Vega’s existing reactive geometry for this purpose.
Proposal
This PR adds the following properties to the text mark definition:
bgTypebgColorbgOpacitybgPaddingbgCornerRadiusWhen any of these properties are set, a rect layer is generated behind the text in the compiled Vega spec. The images below show the result for these two specs: example 1, example 2.
Status
Work in Progress (WIP)Ready for review - feedback requested on the following points:
aspectfor image marks andtensionfor line marks, but it seems these are allowed in other mark definitions as well:"mark": {"type":"bar", "aspect":true, "tension": 1}.Any suggestions or insights would be greatly appreciated!