#1329 Improvement of Beijing subway stations#1330
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances Beijing subway station components by changing the transfer icon design and adding minor text offset controls for fine-tuning station name positioning. The changes include updates to both basic and interchange station types, with comprehensive internationalization support and a save version migration to ensure backwards compatibility.
Changes:
- Updated the transfer icon path (PATH_ARROW) in Beijing subway interchange stations to a new design
- Added minorOffsetX and minorOffsetY attributes to both bjsubway-basic and bjsubway-int station types for precise station name positioning
- Incremented save version from 68 to 69 with migration logic for existing projects
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/save.ts | Bumped save version to 69 and added migration logic for new minorOffset attributes |
| src/util/save.test.ts | Added unit test for the 68→69 save version upgrade |
| src/components/svgs/stations/bjsubway-int.tsx | Updated transfer icon, added minorOffset functionality with UI controls |
| src/components/svgs/stations/bjsubway-basic.tsx | Added minorOffset functionality with UI controls |
| src/i18n/translations/en.json | Added English translations for minorOffset labels |
| src/i18n/translations/zh-Hans.json | Added Simplified Chinese translations for minorOffset labels |
| src/i18n/translations/zh-Hant.json | Added Traditional Chinese translations for minorOffset labels (incomplete) |
| src/i18n/translations/ko.json | Added Korean translations for minorOffset labels |
| src/i18n/translations/ja.json | Added Japanese translations for minorOffset labels |
src/i18n/translations/zh-Hant.json
Outdated
| "labelX": "Minor names offset X", | ||
| "labelY": "Minor names offset Y", |
There was a problem hiding this comment.
Translation inconsistency in Traditional Chinese. For the bjsubwayBasic station, the labelX and labelY are not translated to Traditional Chinese. They remain in English as "Minor names offset X" and "Minor names offset Y", while the bjsubwayInt station has proper Traditional Chinese translations ("站名橫向微小位移" and "站名縱向微小位移"). These should be translated consistently.
| "labelX": "Minor names offset X", | |
| "labelY": "Minor names offset Y", | |
| "labelX": "站名橫向微小位移", | |
| "labelY": "站名縱向微小位移", |
| selections={[ | ||
| { | ||
| label: t('panel.details.stations.bjsubwayInt.minorOffset.-1'), | ||
| value: '-1', | ||
| }, | ||
| { | ||
| label: t('panel.details.stations.bjsubwayInt.minorOffset.0'), | ||
| value: '0', | ||
| }, | ||
| { | ||
| label: t('panel.details.stations.bjsubwayInt.minorOffset.1'), | ||
| value: '1', | ||
| }, | ||
| ]} |
There was a problem hiding this comment.
Inconsistency between X and Y offset options. The minorOffsetX button group offers 5 options (-2, -1, 0, 1, 2) while minorOffsetY only offers 3 options (-1, 0, 1). While this might be intentional based on design requirements, the TypeScript interface at lines 169-170 defines both as supporting the full range '-2' | '-1' | '0' | '1' | '2', which could lead to confusion or runtime issues if users expect parity between X and Y options. Consider either documenting why Y has fewer options or updating the type definition to reflect the actual available options.
| selections={[ | ||
| { | ||
| label: t('panel.details.stations.bjsubwayBasic.minorOffset.-1'), | ||
| value: '-1', | ||
| }, | ||
| { | ||
| label: t('panel.details.stations.bjsubwayBasic.minorOffset.0'), | ||
| value: '0', | ||
| }, | ||
| { | ||
| label: t('panel.details.stations.bjsubwayBasic.minorOffset.1'), | ||
| value: '1', | ||
| }, | ||
| ]} |
There was a problem hiding this comment.
Inconsistency between X and Y offset options. The minorOffsetX button group offers 5 options (-2, -1, 0, 1, 2) while minorOffsetY only offers 3 options (-1, 0, 1). While this might be intentional based on design requirements, the TypeScript interface at lines 147-148 defines both as supporting the full range '-2' | '-1' | '0' | '1' | '2', which could lead to confusion or runtime issues if users expect parity between X and Y options. Consider either documenting why Y has fewer options or updating the type definition to reflect the actual available options.
| if (typeof attr.minorOffsetX !== 'number') { | ||
| attr.minorOffsetX = '0'; | ||
| graph.mergeNodeAttributes(node, { [type]: attr }); | ||
| } | ||
| if (typeof attr.minorOffsetY !== 'number') { |
There was a problem hiding this comment.
Type mismatch in the migration logic. The code sets minorOffsetX to the string '0' when it should be the number 0, but then checks if the type is NOT 'number'. This means the value will always be set to '0' (string) even when it should remain undefined or be converted to the correct type.
According to the BjsubwayBasicStationAttributes and BjsubwayIntStationAttributes interfaces, minorOffsetX should be of type '-2' | '-1' | '0' | '1' | '2' (a string union type), not a number. The condition should check typeof attr.minorOffsetX !== 'string' or check if the attribute is undefined.
| if (typeof attr.minorOffsetX !== 'number') { | |
| attr.minorOffsetX = '0'; | |
| graph.mergeNodeAttributes(node, { [type]: attr }); | |
| } | |
| if (typeof attr.minorOffsetY !== 'number') { | |
| if (typeof attr.minorOffsetX !== 'string') { | |
| attr.minorOffsetX = '0'; | |
| graph.mergeNodeAttributes(node, { [type]: attr }); | |
| } | |
| if (typeof attr.minorOffsetY !== 'string') { |
| if (typeof attr.minorOffsetX !== 'number') { | ||
| attr.minorOffsetX = '0'; | ||
| graph.mergeNodeAttributes(node, { [type]: attr }); | ||
| } | ||
| if (typeof attr.minorOffsetY !== 'number') { |
There was a problem hiding this comment.
Same type mismatch issue as with minorOffsetX. The code sets minorOffsetY to the string '0' but checks if the type is NOT 'number'. According to the TypeScript interface, minorOffsetY should be of type '-2' | '-1' | '0' | '1' | '2' (a string union type), not a number. The condition should check typeof attr.minorOffsetY !== 'string' or check if the attribute is undefined.
| if (typeof attr.minorOffsetX !== 'number') { | |
| attr.minorOffsetX = '0'; | |
| graph.mergeNodeAttributes(node, { [type]: attr }); | |
| } | |
| if (typeof attr.minorOffsetY !== 'number') { | |
| if (typeof attr.minorOffsetX !== 'string') { | |
| attr.minorOffsetX = '0'; | |
| graph.mergeNodeAttributes(node, { [type]: attr }); | |
| } | |
| if (typeof attr.minorOffsetY !== 'string') { |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
1 participant
fix #1329