Fix Border not Working with Text Layer

[FlandiaYingman]

Fixes #236

I haven't run the commit hooks (test:unit, test:smoke, etc.), since the syntax of setting scoped environment variable (ENV=XXX command) doesn't work on windows. See https://stackoverflow.com/questions/25112510/how-to-set-environment-variables-from-within-package-json/27090755#27090755 and its comments for more details. I suggest using https://www.npmjs.com/package/cross-env to deal with this problem.

[clabe45]

Hi, sorry for the late reply. Just started a new job.

Thanks for letting me know about the commit hooks. Could you please open an issue for that?

Reviewing the PR now...

[clabe45]

Two things:

• Make sure val(this, 'border', this.currentTime) is not null before drawing the outline (it can be null).
• The entire text is white in the example, but it should be blue with a white outline.

[clabe45]

The checks passed, but until the commit hook issue gets fixed, you can manually run them on your local machine:

npm run fix
npm run test:unit
npm run test:smoke
npm run test:integration

[clabe45]

Looking at this further, I'd say we should add a separate property textBorder, since border specifies how the outline of the layer's bounds should be rendered. In another issue/PR, we should also deprecate color and rename it to textColor. Thoughts?

[FlandiaYingman]

Actually, the "commit hooks problem" doesn't relate to commit hooks. I have created a issue about it #240.

[FlandiaYingman]

Two things:

• Make sure val(this, 'border', this.currentTime) is not null before drawing the outline (it can be null).
• The entire text is white in the example, but it should be blue with a white outline.

I will try to fix those problems.

[FlandiaYingman]

Looking at this further, I'd say we should add a separate property textBorder, since border specifies how the outline of the layer's bounds should be rendered. In another issue/PR, we should also deprecate color and rename it to textColor. Thoughts?

If I understand you correctly, you meant:

Then adding a separate property would be a good idea, since it allows users to assign different styles on "border" and "textBorder". But I'd say the new property name should be stroke, since it is unified with CanvasRenderingContext2D's strokeText() method. Also, following the convention, we usually use the word "border" to refer to something that is a rectangle, and use the work "stroke" to refer to something whose shape is irregular. You may google "text border" and "text stroke" to check which one is more conventional.

[clabe45]

Makes sense, thanks for opening the issue.

Yeah, that is the distinction I was making. Good point, stroke would be a better name. Glad you pointed that out!

Please also be sure to type stroke as an object, with the same fields as border:

• color - dynamic color object
• thickness - dynamic number

See this article for more info on dynamic properties.

Let me know if anything is unclear or doesn't look quite right.

[clabe45]

Correction:

stroke should be a dynamic object:

Dynamic<{
  color: Color
  thickness?: number
}>

[clabe45]

This looks great!! Good idea on how to implement the position.

One thing though, can you make position an enum? textDirection should be an enum as well, but that can wait for another issue/PR.

[clabe45]

Looks great, merging. Thanks again!!