-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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): Texts 1st patch #8420
Conversation
Build Stats
|
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.
This is it
I would like to spilt up Text in a follow up into:
- renderer
- measurer
QUnit.module('fabric.Text'); | ||
QUnit.module('fabric.Text', { | ||
before() { | ||
fabric.config.configure({ NUM_FRACTION_DIGITS: 2 }); |
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 have no idea why this is needed
w/o tests fail
I will inspect
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.
all the tests are usually done with NUM_FRACTION_DIGITS that was the old defaults to avoid large exports and approximation error between js implementations
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 pointed this out since it was I change I made to tests for them to pass, before they were passing without NUM_FRACTION_DIGITS
assignment, not sure if it was pollution or what
* @param {Number} selectionStart | ||
* @param {Boolean} [skipWrapping] consider the location for unwrapped lines. useful to manage styles. | ||
*/ | ||
get2DCursorLocation(selectionStart: number, skipWrapping?: boolean) { |
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.
migrated from style mixin
argh 3000 lines. let's start. |
src/shapes/text.class.ts
Outdated
protected __skipDimension: boolean; | ||
protected textLines: string[]; | ||
protected _textLines: string[][]; | ||
protected _unwrappedTextLines: string[][]; | ||
protected _text: string[]; | ||
protected cursorWidth: number; | ||
protected __lineHeights: number[]; | ||
protected __lineWidths: number[]; | ||
protected _forceClearCache: boolean; |
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.
some of those are useful to read in certain cases. I'm not sure we should flag them as protected as part of the migration
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 can remove. I have no opinion on this
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 about to commit some comments in the source code and a forgot var/const let's not overlap. i m about to merge this
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
Motivation
Description
Changes
Moved selectionStart/End logic outside of TextStyleMixin to IText where it belongs
Gist
I migrated IText, Textbox as well since it made sense
Next PR I will handle IText mixins
In Action