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.

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

fix: ellipses displaying even when the text does not exceed the limit #2

Merged
merged 4 commits into from
Apr 21, 2024

Conversation

forzgc
Copy link
Contributor

@forzgc forzgc commented Apr 20, 2024

  1. 优化了二分法测量文本宽度的算法,减少了一些不需要的测量
  2. 对width参数加了校验
  3. 修复了 lineClamp 的省略号在文本未超出时也显示的问题

@inottn inottn changed the title 优化getAllLines函数 fix: ellipses displaying even when the text does not exceed the limit Apr 21, 2024
src/index.ts Outdated
Comment on lines 223 to 225
if (width == void 0) {
return [content];
}
Copy link
Owner

Choose a reason for hiding this comment

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

这段逻辑应该不会触发。

src/index.ts Outdated
right = mid;
mid = left + ((mid - left) >> 1);
} else if (measureWidth < width) {
left = mid;
Copy link
Owner

Choose a reason for hiding this comment

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

可以考虑修改 binarySearch 函数的实现,因为 getAllLines 方法不关注 binarySearch 具体的实现逻辑,对应的逻辑拆分会让代码看上去会更清晰一点。

Copy link
Contributor Author

@forzgc forzgc Apr 22, 2024

Choose a reason for hiding this comment

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

没使用 binarySearch 是考虑到两点

一是如果文本只够一行的话,binarySearch 会测量多次才能得到结果,考虑到海报中单行文本比较多的情况,所以希望能先进行一次全部文本的测量

二是 binarySearch 每次调用的时候传入的content都是完整的,而已经push进lines的那部分文本其实不需要在参与进来,除了会导致测量次数增加之外,还会有测量空字符串的情况

原因是 content.slice(index, end + 1) 中 end + 1可能小于 index

binarySearch(
          content,
          (end) =>
            context.measureText(content.slice(index, end + 1)).width > width!,
        ) + 1;

因为这两个原因,改动后的代码我不确定是否通用,所以直接写在了getAllLines中

@inottn inottn merged commit 4725cff into inottn:main Apr 21, 2024
2 checks passed
@inottn
Copy link
Owner

inottn commented Apr 21, 2024

倾向于在一个 PR 内做一个改动,所以这个 PR 先合并了,感谢你的贡献~ 🎉

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