-
Notifications
You must be signed in to change notification settings - Fork 592
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
Disable default strokeDash for legend #1344
Conversation
@@ -30,8 +30,7 @@ export function buildModel(spec: Spec, parent: Model, parentGivenName: string): | |||
} | |||
|
|||
// TODO: figure if we really need opacity in both | |||
export const STROKE_CONFIG = ['stroke', 'strokeWidth', | |||
'strokeDash', 'strokeDashOffset', 'strokeOpacity', 'opacity']; | |||
export const STROKE_CONFIG = ['stroke', 'strokeWidth', 'strokeOpacity', 'opacity']; |
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 think this is the right fix. This would make strokeDash unused for both legend and marks.
Why don't you modify the Line 128 in legend.ts like I suggested?
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 did this before I got your suggestion. I will fix it.
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.
Oh I see. Sounds good then ;)
Fixed it by eliminating the default strokeDash and strokeDashOffset for all marks |
@@ -146,9 +146,9 @@ export namespace properties { | |||
applyMarkConfig(symbols, model, | |||
channel === COLOR ? | |||
/* For color's legend, do not set fill (when filled) or stroke (when unfilled) property from config because the the legend's `fill` or `stroke` scale should have precedence */ | |||
without(FILL_STROKE_CONFIG, [ filled ? 'fill' : 'stroke']) : | |||
without(FILL_STROKE_CONFIG, [ filled ? 'fill' : 'stroke', 'strokeDash', 'strokeDashOffset']) : |
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 would organize this chunk of the code a bit so that the comments still make sense.
Ideally I would do something like:
let config = channel === COLOR ?
/* For color's legend, do not set fill (when filled) or stroke (when unfilled) property from config because the the legend's `fill` or `stroke` scale should have precedence */
... :
/* For other legend, no need to omit. */
...;
config = without(config, ['strokeDash', 'strokeDashOffset'])
applyMarkConfig(symbols, model, config)
Fixed the style. |
Can you add test? (and for future PRs, please add screenshot to help reviewers review PR easier.) |
tests and screenshot are added |
Fix #1314
Now legend won't take
strokeDash
andstrokeDashOffset
from config for all marks.the following spec create the correct legend symbol now.