-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(pattern): remove non-required properties from PatternObject and fix some type issues. #759
Conversation
…ix some type issues.
src/graphic/Pattern.ts
Outdated
@@ -5,16 +5,16 @@ type CanvasPatternRepeat = 'repeat' | 'repeat-x' | 'repeat-y' | 'no-repeat' | |||
export interface PatternObject { | |||
id?: number | |||
|
|||
type: 'pattern' | |||
type?: 'pattern' | |||
|
|||
image: ImageLike | string |
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.
Is it better to use union of interface
so we can have type narrowing?
interface CanvasPatternObject {
type: 'pattern'
image: ImageLike | string
repeat: ...
}
interface SVGPattenObject {
type: 'pattern'
svgElement?: SVGElement
....
}
type PatternObject = CanvasPatternObject | SVGPatternObject
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 which way is better,
- Use union types (you mentioned above)
- Define a parent type named
PatternObject
,CanvasPatternObject
andSVGPatternObject
extend it.
GradientObject is like the second way, but one comment is here that states you are also not sure about this.
// TODO Should GradientObject been LinearGradientObject | RadialGradientObject
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.
Union types provide a more precise type checking for the developers. It's usually better when the subtypes are enumerable.
For Gradient
I now prefer the interface to be:
export interface GradientObjectBase {
id?: number
type: string
colorStops: GradientColorStop[]
global: boolean
}
export interface LinearGradientObject extends GradientObjectBase { };
export interface RadiaGradientObject extends GradientObjectBase { };
export type GradientObject = LinearGradientObject | RadiaGradientObject;
So the type system can know it is a radial gradient is immediately developers add a property r
.
// TypeScript can narrow the type of right-hand object to RadiaGradientObject because it has property `r`.
// It will throw type error when we add properties that belongs to LinearGradientObject like `x2`
const gradient: GradientObject = { r: xxxx }
src/svg/helper/PatternManager.ts
Outdated
@@ -122,7 +122,7 @@ export default class PatternManager extends Definable { | |||
* @param pattern zr pattern instance | |||
* @param patternDom DOM to update | |||
*/ | |||
updateDom(pattern: PatternObject, patternDom: SVGElement) { | |||
updateDom(pattern: SVGPatternObject, patternDom: SVGElement) { |
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.
Sorry for the confusing name CanvasPatterObject
. The difference between these two pattern object is whether to use an image in the pattern or use an SVG element. Both of them are supported in SVG renderer.
zrender/src/svg/helper/PatternManager.ts
Line 139 in d30bcf7
const prevImage = patternDom.getElementsByTagName('image'); |
So perhaps using ImagePatternObject
and SVGPatternObject
is clearer.
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.
Not sure if 3c2fb9a understands what you mean.
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.
And SVGPatternObject
should not have image
property.
…nge `PatternObject` to `ImagePatternObject` in canvas painter.
fixes apache/echarts#14953
type
,svgElement
,svgWidth
,svgHeight
ofPatternObject
optional.