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
chore(TS): polish text #8489
chore(TS): polish text #8489
Conversation
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.
Text is very messy
This PR is another step forward
extracted getMeasuringContext
to top level
@@ -608,7 +592,7 @@ export class Text< | |||
charStyle: any, | |||
forMeasuring?: boolean | |||
) { | |||
ctx.textBaseline = 'alphabetical'; | |||
ctx.textBaseline = 'alphabetic'; |
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.
Thanks to TS.
I am pretty sure this fixes an open bug
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/textBaseline
Build Stats
|
src/shapes/text.class.ts
Outdated
fabric._measuringContext = | ||
(this.canvas && this.canvas.contextCache) || | ||
createCanvasElement().getContext('2d'); | ||
this.pathSegmentsInfo = getPathSegmentsInfo(path.path); |
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 really prefer this to stay in path. It makes easier to swap paths and also the path can use those segment infos for other stuff in the future, since this is info of the path itself.
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.
legit
so we type it on path? or we type text with
path: Path & { info: WhatEver }
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 typed in on path, if the build pass i ll merge this PR
top, | ||
lineHeight | ||
); | ||
this._renderChar(method, ctx, lineIndex, 0, line.join(''), left, top); |
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.
there is some weird reason why lineHeight was there, i ll check it and report back
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 wasn't sure, saw it wasn't in the signature any longer
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.
it could be it was put there to ease some commone override issue but wasn't documented.
I m trying to find out.
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.
We can change the signature to a destructed param, then adding/removing a key is no biggy in terms of subclassing/overriding
length: number; | ||
}; | ||
|
||
export type PathSegmentInfo = { |
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.
we are not doing types with T anymore.
I think is good because it helps understand when is a type and when is class, on top of that i press T
and i see 99% only types in the autocomplete.
At some point we ll have to sort this out
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.
Correct
I am not for and not entirely against.
The only true thing I dislike are names starting with T, then it is a bit weird: TTransformEvent
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.
Then names like TEvent
might be confusing since we might have wanted to abbreviate TransformEvent
into TEvent
@ShaMan123 i ll try to do some little work now and come back to this PR, maybe i can fix the pathInfo myself and merge. |
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
Motivation
Description
Changes
Gist
In Action