Skip to content

Conversation

@canstand
Copy link
Contributor

Description

While preparing #10606, I found that the existing tests were not accurate enough, so I added some edge cases.

@codesandbox
Copy link

codesandbox bot commented May 28, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@canstand canstand marked this pull request as ready for review May 28, 2025 09:42
@asturur
Copy link
Member

asturur commented May 29, 2025

oh this is great thank you. I didn't forget about the segmenter, is just a busy period.
As you see there is a bunch of activity on the maintainance side of things

expect(offset3, 'it returns always 1').toBe(1);
const expected = {
lines: ['aaa', 'aaaaaa ', ' ', 'a', 'aaaaaaa', 'aaaaa', ' aaa'],
hardBreaks: [1, 1, 1, 1, 1, 1, 1], // Note: currently, lineIndex 2 and 4 no hardBreak but still removed a space
Copy link
Member

Choose a reason for hiding this comment

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

i remember this 1 was fixing a bug that was in fabric since long.
Can we write here, this has to be 1 and we need to be careful if this condition changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I added this comment because the test logic here needs to be modified if we want to support text with non-space line breaks. Some thoughts updated in #10606.

@github-actions
Copy link
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 916.091 (0) 301.478 (0)

@asturur asturur merged commit b5120ec into fabricjs:master May 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants