[codex] Support radial label rotation in chord chart#21597
[codex] Support radial label rotation in chord chart#21597
Conversation
Chord labels accepted the shared label.rotate shape but rendered every node label horizontally. This wires radial rotation into the chord label placement path and keeps numeric rotation working through the same branch, with a regression test that inspects the rendered node text rotation. Constraint: Issue #21199 asks specifically for label.rotate: 'radial' on chord labels. Rejected: Reuse pie label layout wholesale | chord labels do not use pie guide-line collision layout and only need the sector-midline rotation rule. Confidence: high Scope-risk: narrow Directive: Keep chord radial rotation aligned with pie's upright text flip unless chord label geometry changes. Tested: npx jest --config test/ut/jest.config.cjs --runTestsByPath test/ut/spec/series/chord.test.ts --coverage=false Tested: npm test -- --runInBand --coverage=false Tested: npm run checktype Tested: npm run lint -- --no-cache src/chart/chord/ChordPiece.ts src/chart/chord/ChordSeries.ts Tested: npx eslint --no-cache test/ut/spec/series/chord.test.ts Tested: npx tsc -p test/ut/tsconfig.json --noEmit --moduleResolution node --skipLibCheck Not-tested: npm run checkheader currently fails on pre-existing header gaps in test/ut/coverage/*, test/line-area-empty-dimension-name.html, and test/visualmap-seriesTargets.html.
|
Thanks for your contribution! The pull request is marked to be To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: This message is shown because the PR description doesn't contain the document related template. |
|
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-21597@3247907 |
There was a problem hiding this comment.
Pull request overview
This PR adds chord-series support for radial node label rotation and updates the chord label layout path so numeric label.rotate values are interpreted in degrees, matching the rest of the codebase. It also adds a new unit test around chord label rotation behavior to cover the feature requested in issue #21199.
Changes:
- Add
'radial'as an allowed chord nodelabel.rotatevalue in the series typings. - Apply chord label rotation at render time, including degree-to-radian conversion for numeric
label.rotate. - Add a unit test that verifies chord node text receives the expected radial rotation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/chart/chord/ChordSeries.ts |
Expands chord node label typings to allow 'radial' rotation. |
src/chart/chord/ChordPiece.ts |
Implements runtime label rotation for chord node labels, including numeric conversion. |
test/ut/spec/series/chord.test.ts |
Adds regression coverage for radial chord label rotation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot pointed out that series-level emphasis, blur, and select label options still used the generic label type after chord labels gained radial rotation. Aligning those state label types with ChordNodeLabelOption keeps the API surface consistent, and a numeric rotation test protects the degree-to-radian path touched by the implementation. Constraint: Chord node labels now support both numeric and radial rotation. Rejected: Leave state label typing generic | it keeps rotate: 'radial' rejected in valid chord state options. Confidence: high Scope-risk: narrow Tested: npx jest --config test/ut/jest.config.cjs --runTestsByPath test/ut/spec/series/chord.test.ts --coverage=false Tested: npm run checktype Tested: npx eslint --no-cache src/chart/chord/ChordSeries.ts test/ut/spec/series/chord.test.ts Tested: npx tsc -p test/ut/tsconfig.json --noEmit --moduleResolution node --skipLibCheck Tested: npm test -- --runInBand --coverage=false
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Chord labels are standalone Text elements, so the shared label styling path does not carry rotate into emphasis, blur, or select transforms. Reuse the chord rotation calculation for normal labels and only add state rotation overrides when those state labels explicitly configure rotate. Constraint: Chord node labels are positioned directly on Text rather than through host textConfig. Rejected: Move chord labels back to host textConfig | would broaden the layout change beyond the review feedback. Confidence: high Scope-risk: narrow Tested: npx jest --config test/ut/jest.config.cjs --runTestsByPath test/ut/spec/series/chord.test.ts --coverage=false Tested: npx eslint --no-cache src/chart/chord/ChordPiece.ts test/ut/spec/series/chord.test.ts Tested: npm run checktype
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ChordPiece instances are reused across option updates, so a state label rotation must be refreshed every render. When a state no longer defines label.rotate, write the current normal rotation into that text state so stale state transforms do not survive later setOption calls. Constraint: Chord labels store rotation directly on standalone Text states. Rejected: Leave state rotation unset when missing | reused Text states can retain an older explicit rotation. Confidence: high Scope-risk: narrow Tested: npx jest --config test/ut/jest.config.cjs --runTestsByPath test/ut/spec/series/chord.test.ts --coverage=false Tested: npx eslint --no-cache src/chart/chord/ChordPiece.ts test/ut/spec/series/chord.test.ts Tested: npm run checktype
Adopt the Copilot review suggestion for state label rotation cleanup. A missing state-specific rotate now writes null into the text state so zrender restores the normal rotation instead of keeping an old override. Constraint: zrender treats null state transform props as absent and restores normal state values. Rejected: Copy normal rotation into every state | works visually but obscures the intended state fallback semantics. Confidence: high Scope-risk: narrow Tested: npx jest --config test/ut/jest.config.cjs --runTestsByPath test/ut/spec/series/chord.test.ts --coverage=false Tested: npx eslint --no-cache src/chart/chord/ChordPiece.ts test/ut/spec/series/chord.test.ts Tested: npm run checktype
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The PR had unit coverage for radial, numeric, and state-specific chord label rotation. This adds a focused HTML regression page with project testHelper assertions so reviewers can inspect the rendered browser case. Constraint: Reviewer feedback expects browser-level HTML cases for PR behavior changes Confidence: high Scope-risk: narrow Tested: npm test -- --runInBand test/ut/spec/series/chord.test.ts Tested: Browser loaded test/chord-label-rotate.html after dev-fast bundle and showed two checked: Pass assertions
Summary
label.rotate: 'radial'support for chord node labels.label.rotateworking by applying degree-to-radian conversion in the chord label layout path.Fixes #21199.
Validation
npx jest --config test/ut/jest.config.cjs --runTestsByPath test/ut/spec/series/chord.test.ts --coverage=falsenpm test -- --runInBand --coverage=falsenpm run checktypenpm run lint -- --no-cache src/chart/chord/ChordPiece.ts src/chart/chord/ChordSeries.tsnpx eslint --no-cache test/ut/spec/series/chord.test.tsnpx tsc -p test/ut/tsconfig.json --noEmit --moduleResolution node --skipLibCheckNote
npm run checkheadercurrently fails on pre-existing files outside this change (test/ut/coverage/*,test/line-area-empty-dimension-name.html, andtest/visualmap-seriesTargets.html). The files changed in this PR have license headers.ScreenShot