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

chore(TS) Add more types to Text, BREAKING: change return type of _getStyleDeclaration #9018

Merged
merged 16 commits into from
Jun 19, 2023
Merged

chore(TS) Add more types to Text, BREAKING: change return type of _getStyleDeclaration #9018

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
8456471
i did this
asturur Jun 14, 2023
fd44b22
more changes
asturur Jun 14, 2023
4c59ab8
fixed tests
asturur Jun 15, 2023
3873c95
Merge branch 'master' into text-type-more
asturur Jun 15, 2023
d34b151
updated master
asturur Jun 15, 2023
b6de41b
import type
asturur Jun 15, 2023
7e209cd
fabric imports
asturur Jun 15, 2023
8b1b41e
test this
asturur Jun 15, 2023
ee32043
removed a bunch of small ts-nocheck
asturur Jun 15, 2023
c3a8869
seems ok
asturur Jun 15, 2023
316497c
Update src/util/dom_style.ts
asturur Jun 16, 2023
33a5eeb
Merge branch 'master' into text-type-more
asturur Jun 16, 2023
a3f55fa
Merge branch 'master' into text-type-more
asturur Jun 17, 2023
c7fffcf
changes
asturur Jun 17, 2023
bc573e8
no test changes
asturur Jun 17, 2023
c3e8a0e
Merge branch 'master' into text-type-more
asturur Jun 19, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- chore(TS): Remove @ts-nocheck from Text class. [#9018](https://github.com/fabricjs/fabric.js/pull/9018)
- fix(IText): `exitEditing` should clear contextTop [#9020](https://github.com/fabricjs/fabric.js/pull/9020)
- ci(): prettier after changelog action [#9021](https://github.com/fabricjs/fabric.js/pull/9021)

Expand Down
20 changes: 14 additions & 6 deletions src/shapes/IText/IText.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,10 +497,14 @@ export class IText<
lineIndex = cursorLocation.lineIndex,
charIndex =
cursorLocation.charIndex > 0 ? cursorLocation.charIndex - 1 : 0,
charHeight = this.getValueOfPropertyAt(lineIndex, charIndex, 'fontSize'),
charHeight = this.getValueOfPropertyAt<'fontSize'>(
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
lineIndex,
charIndex,
'fontSize'
),
multiplier = this.scaleX * this.canvas!.getZoom(),
cursorWidth = this.cursorWidth / multiplier,
dy = this.getValueOfPropertyAt(lineIndex, charIndex, 'deltaY'),
dy = this.getValueOfPropertyAt<'deltaY'>(lineIndex, charIndex, 'deltaY'),
topOffset =
boundaries.topOffset +
((1 - this._fontSizeFraction) * this.getHeightOfLine(lineIndex)) /
Expand All @@ -514,7 +518,11 @@ export class IText<
}
ctx.fillStyle =
this.cursorColor ||
this.getValueOfPropertyAt(lineIndex, charIndex, 'fill');
(this.getValueOfPropertyAt<'fill'>(
lineIndex,
charIndex,
'fill'
) as string);
ctx.globalAlpha = this._currentCursorOpacity;
ctx.fillRect(
boundaries.left + boundaries.leftOffset - cursorWidth / 2,
Expand Down Expand Up @@ -657,7 +665,7 @@ export class IText<
*/
getCurrentCharFontSize(): number {
const cp = this._getCurrentCharIndex();
return this.getValueOfPropertyAt(cp.l, cp.c, 'fontSize');
return this.getValueOfPropertyAt<'fontSize'>(cp.l, cp.c, 'fontSize');
}

/**
Expand All @@ -668,9 +676,9 @@ export class IText<
* Unused by the library, is for the end user
* @return {String | TFiller} Character color (fill)
*/
getCurrentCharColor(): string | TFiller {
getCurrentCharColor(): string | TFiller | null {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fill and stroke being null is the cause of many issue with types and a lot of extra null checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we removing null why this this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because i m not removing null in this PR.
But this PR brought up the issue again

const cp = this._getCurrentCharIndex();
return this.getValueOfPropertyAt(cp.l, cp.c, 'fill');
return this.getValueOfPropertyAt<'fill'>(cp.l, cp.c, 'fill');
}

/**
Expand Down
7 changes: 5 additions & 2 deletions src/shapes/IText/ITextBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,11 @@ export abstract class ITextBehavior<
lineIndex = cursorLocation.lineIndex,
charIndex = cursorLocation.charIndex,
charHeight =
this.getValueOfPropertyAt(lineIndex, charIndex, 'fontSize') *
this.lineHeight,
this.getValueOfPropertyAt<'fontSize'>(
lineIndex,
charIndex,
'fontSize'
) * this.lineHeight,
leftOffset = boundaries.leftOffset,
retinaScaling = this.getCanvasRetinaScaling(),
upperCanvas = this.canvas.upperCanvasEl,
Expand Down
42 changes: 23 additions & 19 deletions src/shapes/Text/StyledText.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import type { StylePropertiesType } from './constants';
import type { Text } from './Text';
import { pick } from '../../util';

export type TextStyleDeclaration = Partial<Pick<Text, StylePropertiesType>>;
export type CompleteTextStyleDeclaration = Pick<Text, StylePropertiesType>;

export type TextStyleDeclaration = Partial<CompleteTextStyleDeclaration>;

export type TextStyle = {
[line: number | string]: { [char: number | string]: TextStyleDeclaration };
Expand Down Expand Up @@ -179,21 +181,18 @@ export abstract class StyledText<
}
}

private _extendStyles(index: number, styles: TextStyleDeclaration) {
private _extendStyles(index: number, styles: TextStyleDeclaration): void {
const { lineIndex, charIndex } = this.get2DCursorLocation(index);

if (!this._getLineStyle(lineIndex)) {
this._setLineStyle(lineIndex);
}

if (!this._getStyleDeclaration(lineIndex, charIndex)) {
if (!Object.keys(this._getStyleDeclaration(lineIndex, charIndex)).length) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only code change i made.
there were 5 occurrencies in which returning undefined from _getStyleDeclaration needed to be fixed with an || {} while just one where returning undefined ( or null ) was useful, to know if there was no style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is fine but BREAKING if anyone used it before

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would make this breaking, and i someone asks, i would add the hasStyleDeclaration method that return true/false, or maybe i can add it right away

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should
Then there is a clear migration path (Object.keys is pretty clear also but...)

this._setStyleDeclaration(lineIndex, charIndex, {});
}

return Object.assign(
this._getStyleDeclaration(lineIndex, charIndex) || {},
styles
);
Object.assign(this._getStyleDeclaration(lineIndex, charIndex), styles);
}

/**
Expand Down Expand Up @@ -224,11 +223,9 @@ export abstract class StyledText<
*/
getStyleAtPosition(position: number, complete?: boolean) {
const { lineIndex, charIndex } = this.get2DCursorLocation(position);
return (
(complete
? this.getCompleteStyleDeclaration(lineIndex, charIndex)
: this._getStyleDeclaration(lineIndex, charIndex)) || {}
);
return complete
? this.getCompleteStyleDeclaration(lineIndex, charIndex)
: this._getStyleDeclaration(lineIndex, charIndex);
}

/**
Expand All @@ -246,14 +243,18 @@ export abstract class StyledText<
}

/**
* get the reference, not a clone, of the style object for a given character
* get the reference, not a clone, of the style object for a given character,
* if not style is set for a pre det
* @param {Number} lineIndex
* @param {Number} charIndex
* @return {Object} style object
* @return {Object} style object a REFERENCE to the existing one or a new empty object
*/
_getStyleDeclaration(lineIndex: number, charIndex: number) {
_getStyleDeclaration(
lineIndex: number,
charIndex: number
): TextStyleDeclaration {
const lineStyle = this.styles && this.styles[lineIndex];
return lineStyle ? lineStyle[charIndex] : null;
return lineStyle ? lineStyle[charIndex] ?? {} : {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we ok with abbreviations now or not yet?

Suggested change
return lineStyle ? lineStyle[charIndex] ?? {} : {};
return lineStyle?.[charIndex] ?? {};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is so much longer the abbreviation because safari.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i m also not sure about performance in general of that abbrevieation

}

/**
Expand All @@ -263,12 +264,15 @@ export abstract class StyledText<
* @param {Number} charIndex position of the character on the line
* @return {Object} style object
*/
getCompleteStyleDeclaration(lineIndex: number, charIndex: number) {
getCompleteStyleDeclaration(
lineIndex: number,
charIndex: number
): CompleteTextStyleDeclaration {
return {
// @ts-expect-error readonly
...pick(this, (this.constructor as typeof StyledText)._styleProperties),
...(this._getStyleDeclaration(lineIndex, charIndex) || {}),
} as TextStyleDeclaration;
...this._getStyleDeclaration(lineIndex, charIndex),
} as CompleteTextStyleDeclaration;
}

/**
Expand Down
Loading