Skip to content
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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 18, 2021
Merged

fix(pattern): remove non-required properties from PatternObject and fix some type issues. #759

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53ff84d
fix(pattern): remove non-required properties from PatternObject and f…
plainheart May 17, 2021
d30bcf7
fix(pattern): split PatternObject to CanvasPatternObject & SVGPattern…
plainheart May 17, 2021
3c2fb9a
fix(pattern): rename `CanvasPatternObject` to `ImagePatternObject`.
plainheart May 17, 2021
81ffaf6
fix(pattern): remove `image` property from `SVGPatternObject` and cha…
plainheart May 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/canvas/Layer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import CanvasPainter from './Painter';
import { GradientObject, InnerGradientObject } from '../graphic/Gradient';
import { ZRCanvasRenderingContext } from '../core/types';
import Eventful from '../core/Eventful';
import Element, { ElementEventCallback } from '../Element';
import { ElementEventCallback } from '../Element';
import { getCanvasGradient } from './helper';
import { createCanvasPattern } from './graphic';
import Displayable from '../graphic/Displayable';
Expand Down Expand Up @@ -353,7 +353,7 @@ export default class Layer extends Eventful {
}
i++;
}
} while (hasIntersections)
} while (hasIntersections);

this._paintRects = mergedRepaintRects;

Expand Down
1 change: 0 additions & 1 deletion src/canvas/Painter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import Storage from '../Storage';
import { brush, BrushScope, brushSingle } from './graphic';
import { PainterBase } from '../PainterBase';
import BoundingRect from '../core/BoundingRect';
import Element from '../Element';
import IncrementalDisplayable from '../graphic/IncrementalDisplayable';
import Path from '../graphic/Path';
import { REDARAW_BIT } from '../graphic/constants';
Expand Down
1 change: 0 additions & 1 deletion src/canvas/graphic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { DEFAULT_FONT } from '../contain/text';
import { MatrixArray } from '../core/matrix';
import { map } from '../core/util';
import { normalizeLineDash } from '../graphic/helper/dashStyle';
import Element from '../Element';
import IncrementalDisplayable from '../graphic/IncrementalDisplayable';
import { REDARAW_BIT, SHAPE_CHANGED_BIT } from '../graphic/constants';

Expand Down
2 changes: 1 addition & 1 deletion src/graphic/Path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import Displayable, { DisplayableProps,
import Element, { ElementAnimateConfig } from '../Element';
import PathProxy from '../core/PathProxy';
import * as pathContain from '../contain/path';
import Pattern, { PatternObject } from './Pattern';
import { PatternObject } from './Pattern';
import { Dictionary, PropType, MapToType } from '../core/types';
import BoundingRect from '../core/BoundingRect';
import { LinearGradientObject } from './LinearGradient';
Expand Down
8 changes: 4 additions & 4 deletions src/graphic/Pattern.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@pissang pissang May 17, 2021 •
edited
Loading

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

Copy link
Collaborator Author

@plainheart plainheart May 17, 2021 •
edited
Loading

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,

  1. Use union types (you mentioned above)
  2. Define a parent type named PatternObject, CanvasPatternObject and SVGPatternObject 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

Copy link
Contributor

@pissang pissang May 17, 2021 •
edited
Loading

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 }

/**
* svg element can only be used in svg renderer currently.
* svgWidth, svgHeight defines width and height used for pattern.
*/
svgElement: SVGElement
svgWidth: number
svgHeight: number
svgElement?: SVGElement
svgWidth?: number
svgHeight?: number

repeat: CanvasPatternRepeat

Expand Down