Open xweiba opened 4 months ago
Is it possible to push this on: https://github.com/html2canvas/html2canvas
Is it possible to push this on:是否有可能推动这个: https://github.com/html2canvas/html2canvas
Already PR
Review Summary 评测总结
The changes to the
CanvasRenderer.js
file include updates to how text decorations are drawn on a canvas. These changes introduce a variablefillHeight
to adjust the height of the text decoration lines dynamically and aim to fix issues related to high-resolution devices. The code appears to address previous comments and TODOs by streamlining the drawing process.对CanvasRenderer.js
文件的更改包括对如何在画布上绘制文本装饰的更新。这些更改引入了变量fillHeight
来动态调整文本装饰线的高度,旨在解决与高分辨率设备相关的问题。该代码似乎通过简化绘图过程来解决以前的注释和 TODO。Key Changes 主要变化
Variable Introduction: 变量介绍:
- Added
var fillHeight = 1;
to standardize the fill height of the text decorations.添加了var fillHeight = 1;
以标准化文本装饰的填充高度。Updated Drawing Logic: 更新绘图逻辑:
- Simplified the drawing logic for
TEXT_DECORATION_LINE_UNDERLINE
,TEXT_DECORATION_LINE_OVERLINE
, andTEXT_DECORATION_LINE_LINE_THROUGH
to use the newfillHeight
variable.简化了TEXT_DECORATION_LINE_UNDERLINE
、TEXT_DECORATION_LINE_OVERLINE
和TEXT_DECORATION_LINE_LINE_THROUGH
的绘制逻辑以使用新的fillHeight
变量。Removed Redundant Code: 删除冗余代码:
- Removed old calculations and TODO comments related to handling font size variations and browser inconsistencies.删除了与处理字体大小变化和浏览器不一致相关的旧计算和 TODO 注释。
Recommendations 建议
The changes improve the readability and maintainability of the code. However, here are a few recommendations:这些更改提高了代码的可读性和可维护性。不过,这里有一些建议:
- Variable Naming: Consider using a more descriptive name for
fillHeight
, such asdecorationLineHeight
, to clarify its purpose.变量命名:考虑为fillHeight
使用更具描述性的名称,例如decorationLineHeight
,以阐明其用途。- Code Comments: Although some TODO comments were removed, it might be beneficial to add comments explaining why
fillHeight
is used and how it resolves the previous issues.代码注释:虽然删除了一些 TODO 注释,但添加注释来解释为什么使用fillHeight
以及它如何解决以前的问题可能会有所帮助。- Testing: Ensure thorough testing across different browsers and devices to confirm that the new approach consistently resolves the high-resolution device issues and maintains correct text decoration rendering.测试:确保在不同的浏览器和设备上进行彻底的测试,以确认新方法能够一致地解决高分辨率设备问题并保持正确的文本装饰渲染。
Example with Recommendations Applied
应用建议的示例 Here's a slightly modified version with a more descriptive variable name and an added comment:这是一个稍作修改的版本,具有更具描述性的变量名称和添加的注释:
export class CanvasRenderer extends Renderer { ... if (styles.textDecorationLine.length) { this.ctx.fillStyle = asString(styles.textDecorationColor || styles.color); styles.textDecorationLine.forEach((textDecorationLine) => { var decorationLineHeight = 1; // Fix for high-resolution device issues switch (textDecorationLine) { case TEXT_DECORATION_LINE_UNDERLINE: this.ctx.fillRect(text.bounds.left, text.bounds.top + text.bounds.height - decorationLineHeight, text.bounds.width, decorationLineHeight); break; case TEXT_DECORATION_LINE_OVERLINE: this.ctx.fillRect(text.bounds.left, text.bounds.top, text.bounds.width, decorationLineHeight); break; case TEXT_DECORATION_LINE_LINE_THROUGH: this.ctx.fillRect(text.bounds.left, text.bounds.top + (text.bounds.height / 2) - (decorationLineHeight / 2), text.bounds.width, decorationLineHeight); break; } }); } ... }
Review Summary 评测总结
The changes to the
CanvasRenderer.js
file include updates to how text decorations are drawn on a canvas. These changes introduce a variablefillHeight
to adjust the height of the text decoration lines dynamically and aim to fix issues related to high-resolution devices. The code appears to address previous comments and TODOs by streamlining the drawing process.对CanvasRenderer.js
文件的更改包括对如何在画布上绘制文本装饰的更新。这些更改引入了变量fillHeight
来动态调整文本装饰线的高度,旨在解决与高分辨率设备相关的问题。该代码似乎通过简化绘图过程来解决以前的注释和 TODO。Key Changes 主要变化
Variable Introduction: 变量介绍:
- Added
var fillHeight = 1;
to standardize the fill height of the text decorations.添加了var fillHeight = 1;
以标准化文本装饰的填充高度。Updated Drawing Logic: 更新绘图逻辑:
- Simplified the drawing logic for
TEXT_DECORATION_LINE_UNDERLINE
,TEXT_DECORATION_LINE_OVERLINE
, andTEXT_DECORATION_LINE_LINE_THROUGH
to use the newfillHeight
variable.简化了TEXT_DECORATION_LINE_UNDERLINE
、TEXT_DECORATION_LINE_OVERLINE
和TEXT_DECORATION_LINE_LINE_THROUGH
的绘制逻辑以使用新的fillHeight
变量。Removed Redundant Code: 删除冗余代码:
- Removed old calculations and TODO comments related to handling font size variations and browser inconsistencies.删除了与处理字体大小变化和浏览器不一致相关的旧计算和 TODO 注释。
Recommendations 建议
The changes improve the readability and maintainability of the code. However, here are a few recommendations:这些更改提高了代码的可读性和可维护性。不过,这里有一些建议:
- Variable Naming: Consider using a more descriptive name for
fillHeight
, such asdecorationLineHeight
, to clarify its purpose.变量命名:考虑为fillHeight
使用更具描述性的名称,例如decorationLineHeight
,以阐明其用途。- Code Comments: Although some TODO comments were removed, it might be beneficial to add comments explaining why
fillHeight
is used and how it resolves the previous issues.代码注释:虽然删除了一些 TODO 注释,但添加注释来解释为什么使用fillHeight
以及它如何解决以前的问题可能会有所帮助。- Testing: Ensure thorough testing across different browsers and devices to confirm that the new approach consistently resolves the high-resolution device issues and maintains correct text decoration rendering.测试:确保在不同的浏览器和设备上进行彻底的测试,以确认新方法能够一致地解决高分辨率设备问题并保持正确的文本装饰渲染。
Example with Recommendations Applied
应用建议的示例 Here's a slightly modified version with a more descriptive variable name and an added comment:这是一个稍作修改的版本,具有更具描述性的变量名称和添加的注释:
export class CanvasRenderer extends Renderer { ... if (styles.textDecorationLine.length) { this.ctx.fillStyle = asString(styles.textDecorationColor || styles.color); styles.textDecorationLine.forEach((textDecorationLine) => { var decorationLineHeight = 1; // Fix for high-resolution device issues switch (textDecorationLine) { case TEXT_DECORATION_LINE_UNDERLINE: this.ctx.fillRect(text.bounds.left, text.bounds.top + text.bounds.height - decorationLineHeight, text.bounds.width, decorationLineHeight); break; case TEXT_DECORATION_LINE_OVERLINE: this.ctx.fillRect(text.bounds.left, text.bounds.top, text.bounds.width, decorationLineHeight); break; case TEXT_DECORATION_LINE_LINE_THROUGH: this.ctx.fillRect(text.bounds.left, text.bounds.top + (text.bounds.height / 2) - (decorationLineHeight / 2), text.bounds.width, decorationLineHeight); break; } }); } ... }
Thank you for your suggestion. I have made the modifications accordingly.
build failure: 'middle' is declared but its value is never read. 191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);
build failure: 'middle' is declared but its value is never read.构建失败:声明了“middle”,但从未读取其值。 191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);191 const { 基线, 中间 } = this.fontMetrics.getMetrics(fontFamily, fontSize);
Okay, I have made the changes.
build failure: 'middle' is declared but its value is never read.构建失败:声明了“middle”,但从未读取其值。 191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);191 const { 基线, 中间 } = this.fontMetrics.getMetrics(fontFamily, fontSize);
Okay, I have made the changes.
The original author is not active on github, can you push this on: https://github.com/yorickshan/html2canvas-pro, thx ❤️ (ps: 有什么问题也可以用中文交流 😄 )
build failure: 'middle' is declared but its value is never read.构建失败:声明了“middle”,但从未读取其值。 191 const { baseline, middle } = this.fontMetrics.getMetrics(fontFamily, fontSize);191 const { 基线, 中间 } = this.fontMetrics.getMetrics(fontFamily, fontSize);
Okay, I have made the changes.
The original author is not active on github, can you push this on: https://github.com/yorickshan/html2canvas-pro, thx ❤️ (ps: 有什么问题也可以用中文交流 😄 )
好的 PR了 😄
Summary
This PR fixes/implements the following bugs/features
Closing issues
Fixes #2238 #1624 #2107