Closed FlandiaYingman closed 1 year ago
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...
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
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?
Actually, the "commit hooks problem" doesn't relate to commit hooks. I have created a issue about it #240.
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.
Looking at this further, I'd say we should add a separate property
textBorder
, sinceborder
specifies how the outline of the layer's bounds should be rendered. In another issue/PR, we should also deprecatecolor
and rename it totextColor
. 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.
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 objectthickness
- dynamic numberSee this article for more info on dynamic properties.
Let me know if anything is unclear or doesn't look quite right.
Correction:
stroke
should be a dynamic object:
Dynamic<{
color: Color
thickness?: number
}>
Looks great, merging. Thanks again!!
Fixes https://github.com/etro-js/etro/issues/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.