-
Notifications
You must be signed in to change notification settings - Fork 607
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(#8327): Boxplot set null quantitative to 0 #8409
Conversation
site/_data/versions.yml
Outdated
vega: 5.22.1 | ||
vega-lite: 5.5.0 | ||
vega-embed: 6.21.0 | ||
vega-tooltip: 0.28.0 |
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'm not sure if I should include this file.
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.
yeah, please remove it. It doesn't seem relevant to this PR.
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.
LGTM. Please address these minor comments and feel free to to "squash and merge".
src/compositemark/boxplot.ts
Outdated
@@ -102,6 +103,8 @@ export function normalizeBoxPlot( | |||
config | |||
); | |||
|
|||
const invalid = markDef.invalid || null; //|| null to respect the existed default behavior |
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.
Let's remove || null; //|| null to respect the existed default behavior
since the doc has been saying that filter is the default (for all marks).
So the existing behavior to include null
by default was a bug.
src/mark.ts
Outdated
@@ -75,7 +75,16 @@ export interface TooltipContent { | |||
/** @hidden */ | |||
export type Hide = 'hide'; | |||
|
|||
export interface VLOnlyMarkConfig<ES extends ExprRef | SignalRef> extends ColorMixins<ES> { | |||
export interface MarkInvalid { |
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.
For consistency, please rename to MarkInvalidMixins
.
site/_data/versions.yml
Outdated
vega: 5.22.1 | ||
vega-lite: 5.5.0 | ||
vega-embed: 6.21.0 | ||
vega-tooltip: 0.28.0 |
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.
yeah, please remove it. It doesn't seem relevant to this PR.
fix #8327
π¦ Published PR as canary version:
5.5.1--canary.8409.94b06f4.0
β¨ Test out this PR locally via:
npm install vega-lite@5.5.1--canary.8409.94b06f4.0 # or yarn add vega-lite@5.5.1--canary.8409.94b06f4.0