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
fix: resolve theta to independent by default for faceted charts #7563
Conversation
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.
Thanks for adding this. It'd be nice to also cover relevant cases here in the same PR.
@@ -4,7 +4,9 @@ import {Resolve, ResolveMode} from '../resolve'; | |||
import {isConcatModel, isFacetModel, isLayerModel, Model} from './model'; | |||
|
|||
export function defaultScaleResolve(channel: ScaleChannel, model: Model): ResolveMode { | |||
if (isLayerModel(model) || isFacetModel(model)) { | |||
if (isFacetModel(model)) { | |||
return channel === 'theta' ? 'independent' : 'shared'; |
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.
What about radius? I think radius probably should be independent too.
Similarly, I think theta/concat should not be shared for concat.
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 don't know whether I agree. I think radius is actually more easily comparable across facets so it makes sense to share by default.
I agree that theta and radius should be independent in concat. I'll make that change.
9ecc99f
to
56d4eca
Compare
…#7563) * feat: resolve theta to independent for faceted chars * docs: document resolve for theta in facets * feat: don't share theta and radius in concat add tests * chore: update examples [CI] Co-authored-by: GitHub Actions Bot <vega-actions-bot@users.noreply.github.com>
Thanks for @leibatt for the issue report.