-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat(module:qrcode): support nzType && nzBoostLevel, delete nzPadding #9535
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
base: master
Are you sure you want to change the base?
Conversation
6dea67e to
367b59a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9535 +/- ##
==========================================
+ Coverage 90.06% 90.52% +0.45%
==========================================
Files 570 564 -6
Lines 23379 23003 -376
Branches 4650 4606 -44
==========================================
- Hits 21057 20823 -234
+ Misses 1614 1419 -195
- Partials 708 761 +53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
62ea26c to
83bb521
Compare
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.
Pull Request Overview
This PR refactors the QR code component to support multiple rendering types (canvas and SVG) and introduces a new nzBoostLevel property for automatic error correction optimization. The implementation migrates from the old decorator-based API to Angular's modern signal-based API, splitting the rendering logic into separate canvas and SVG components.
Key changes:
- Introduces
nzTypeinput to switch between 'canvas' and 'svg' rendering - Adds
nzBoostLevelproperty to enable automatic error correction level boosting - Removes the
nzPaddingproperty (breaking change) - Migrates to signal-based inputs/outputs and reactive state management
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| components/qr-code/utils.ts | New utility file providing QR code generation helpers (path generation, module excavation, image settings calculation) |
| components/qr-code/useQRCode.ts | New QR code generation hook that encapsulates the core QR encoding logic |
| components/qr-code/typing.ts | TypeScript type definitions for QR code properties, including new types for rendering options |
| components/qr-code/qrcode.component.ts | Main component refactored to use signals, delegates rendering to child components based on nzType |
| components/qr-code/qrcode-canvas.component.ts | New canvas-based renderer component handling canvas drawing and image overlay |
| components/qr-code/qrcode-svg.component.ts | New SVG-based renderer component handling SVG path generation and image embedding |
| components/qr-code/qrcode.ts | Removed - old implementation replaced by new modular architecture |
| components/qr-code/qrcode.spec.ts | Removed - old tests for formatPadding function no longer applicable |
| components/qr-code/qrcode.module.ts | Updated to import and export new canvas and SVG components |
| components/qr-code/qrcode.component.spec.ts | Added test for new nzType property, updated selector for canvas element |
| components/qr-code/public-api.ts | Exports new typing definitions for external consumption |
| components/qr-code/doc/index.zh-CN.md | Updated documentation with nzType and nzBoostLevel properties, removed nzPadding |
| components/qr-code/doc/index.en-US.md | Updated documentation with nzType and nzBoostLevel properties, removed nzPadding |
| components/qr-code/demo/type.ts | New demo showing canvas vs SVG rendering types |
| components/qr-code/demo/type.md | Documentation for the type demo |
| components/qr-code/demo/padding.md | Removed - padding feature no longer supported |
| selector: 'nz-qrcode-svg', | ||
| exportAs: 'nzQRCodeSVG', | ||
| template: ` | ||
| <svg [attr.height]="size()" [attr.width]="size()" [attr.viewBox]="viewBox" role="img"> |
Copilot
AI
Nov 21, 2025
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.
The SVG element has role="img" but is missing an accessible label. Add an aria-label or allow passing a title input (like the title property defined in QRProps interface in typing.ts) to provide screen readers with meaningful context about the QR code content.
| this.canvas.nativeElement.height = this.canvas.nativeElement.width = this.size() * pixelRatio; | ||
| this.canvas.nativeElement.style.width = this.canvas.nativeElement.style.height = `${this.size()}px`; | ||
|
|
||
| const scale = (this.size() / this.numCells()) * pixelRatio; |
Copilot
AI
Nov 21, 2025
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.
Canvas transformation state issue: ctx.scale() is called on every render without resetting the transform first. This will cause the scale to accumulate on subsequent renders, leading to incorrect rendering. Add ctx.setTransform(1, 0, 0, 1, 0, 0) before scaling to reset the transformation matrix.
| const scale = (this.size() / this.numCells()) * pixelRatio; | |
| const scale = (this.size() / this.numCells()) * pixelRatio; | |
| ctx.setTransform(1, 0, 0, 1, 0, 0); |
| return { | ||
| src: this.nzIcon(), | ||
| x: undefined, | ||
| y: undefined, | ||
| height: this.nzIconSize() ?? 40, | ||
| width: this.nzIconSize() ?? 40, | ||
| excavate: true, | ||
| crossOrigin: 'anonymous' | ||
| }; |
Copilot
AI
Nov 21, 2025
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.
The imageSettings computed signal is recalculated whenever any input changes, but it's always recreated with the same structure. Consider using a more efficient approach or memoization to avoid unnecessary object recreations when the icon-related inputs haven't changed.
| return { | |
| src: this.nzIcon(), | |
| x: undefined, | |
| y: undefined, | |
| height: this.nzIconSize() ?? 40, | |
| width: this.nzIconSize() ?? 40, | |
| excavate: true, | |
| crossOrigin: 'anonymous' | |
| }; | |
| // Memoization: only recreate the object if icon-related inputs change | |
| // Use closure variables to cache previous values and object | |
| type IconInputs = { src: string; size: number }; | |
| // Static closure variables | |
| let prev: IconInputs | undefined; | |
| let cached: ImageSettings | undefined; | |
| const src = this.nzIcon(); | |
| const size = this.nzIconSize() ?? 40; | |
| if (!prev || prev.src !== src || prev.size !== size) { | |
| prev = { src, size }; | |
| cached = { | |
| src, | |
| x: undefined, | |
| y: undefined, | |
| height: size, | |
| width: size, | |
| excavate: true, | |
| crossOrigin: 'anonymous' | |
| }; | |
| } | |
| return cached!; |
83bb521 to
fe281f5
Compare
| @@ -0,0 +1,15 @@ | |||
| --- | |||
| order: 5 | |||
| version: 20.5.0 | |||
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.
20.5.0 is not planned
| import QrSegment = qrcodegen.QrSegment; | ||
| import QrCode = qrcodegen.QrCode; | ||
|
|
||
| export const useQRCode = ( |
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.
useXxx is a react-style name, please change to angular style
| readonly bgColor = input<string>(DEFAULT_BACKGROUND_COLOR); | ||
|
|
||
| constructor() { | ||
| effect(() => { |
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.
what about using ngOnChanges instead?
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.
In signals-based components, ngOnChanges is invalid.
|
|
||
| @NgModule({ | ||
| imports: [NzQRCodeComponent], | ||
| imports: [NzQRCodeComponent, NzQrcodeCanvasComponent, NzQrcodeSvgComponent], |
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.
NzQrcodeCanvasComponent and NzQrcodeSvgComponent are imported in NzQRCodeComponent, they are redundant here.
| description: Convert text into QR codes, and support custom color and logo. | ||
| --- | ||
|
|
||
| > ⚠️ `nzPadding` has been removed in `v20.5.0`. |
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.
我觉得最好先保留 nzPadding,标准的做法是在文档/代码中将其标记为弃用,我们可以在 v22 中正式移除它。
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information