#220 Shanghai Metro Style Station Number #291
Conversation
|
Hi, you need to resolve conflicts before any review. |
Fixed in d458129. |
|
Besides, the i18n of this pull request was not fixed right now. Please check other things without this. |
No problem. I'll focus on the implementation instead of text. CI checks and builds are triggered for you. 😃 |
|
Thinks tests are something wrong. I have prettier the code yet. Because of the VSCode crashed so it breaks the prettier in the shell in workspace? Emmm, I will fix it sometimes then. |
|
OK! Lint fixed in 985254c. I think it can merge. |
|
I'll do a review in the following days. Hope that won't take long. |
|
Merged lasted master and fix indentation |
|
Thanks to help me fix format.@wongchito |
| @@ -4,6 +4,10 @@ | |||
| .rmg-name__en { | |||
| font-family: Arial, sans-serif; | |||
| } | |||
| .rmg-station-name { | |||
There was a problem hiding this comment.
As we said before, no need for another class style, .rmg-name__en would be fine for numbers.
| /** | ||
| * Display the station number in railmap or not. | ||
| */ | ||
| showStationNumberRailmap: boolean; |
There was a problem hiding this comment.
Do we really need to distinguish between two scenarios? For stations that only display station number at runin canvas, I believe that is just an obsolete image at will be replaced soon.
There was a problem hiding this comment.
Like the resolve of Disney Icon. There're some person reason in it (For my Minecraft server), sorry.
There was a problem hiding this comment.
If you need the station number on only one canvas, why not download one canvas and switch the station number button and then download another canvas?
There was a problem hiding this comment.
Because now numbers in railmap were replaced, but at Xinjiangwancheng - Hangzhong Road/Hongqiao Railway Station the station numbers on runin also there.(Xinjiangwancheng removed.)
| </ListItemSecondaryAction> | ||
| </ListItem> | ||
| <ListItem> | ||
| <TextField |
There was a problem hiding this comment.
Will you put a label before the line num? Just like what GTMTR did https://github.com/wongchito/RailMapGenerator/blob/rmg-3.12.3/src/panels/design/list-gzmtr.tsx#L63
There was a problem hiding this comment.
....I forget to do it because there are only one field in part 1.
| <ListItemText primary={t('design.panelType.button')} /> | ||
| <Select native value={panelType} onChange={handleChange} style={{ width: 166 }}> | ||
| {Object.values(PanelTypeShmetro).map(type => ( | ||
| <option key={type} value={type}> |
There was a problem hiding this comment.
Have you tested default info_panel_type sh? It looks a bit weird on my browser.
There was a problem hiding this comment.
Oh! Sorry my test template is using sh2020.
There was a problem hiding this comment.
Just find a problem: Shanghai Line 10 removed all station number on railmap in the 2012 style (which is the current style (without platform number version) that RMG uses) update although the station number still remain on Running-in):
Which now looks like this (the 2020 style also used the normal railmap without station number) (source: https://zhuanlan.zhihu.com/p/57368457)
There was a problem hiding this comment.
@52PD Yes, this is the reason why station number in shmetro has a lower priority. And since this PR is targeted for V3, we may need a rewrite for this part.
There was a problem hiding this comment.
I am going to rewrite it for v5. Thinks that we can make stationNumberRailmap option hidden and we only can edit it in json.
There was a problem hiding this comment.
I'm still insisting that stationNumberRailmap is not needed. Users can hide the station number if showStationNumber is set. They can download one canvas, switch the control, and download another canvas.
| <circle cx={xpos} cy={ypos} r="45" stroke="var(--rmg-grey)" strokeWidth="10" fill="white"></circle> | ||
| <line x1={xpos - 40} y1={ypos} x2={xpos + 40} y2={ypos} stroke="rgb(0,0,0)" strokeWidth="2" /> | ||
| <StationNumberText | ||
| transform={'translate(' + xpos.toString() + ',' + (ypos - 5).toString() + ')'} |
There was a problem hiding this comment.
I'd say transform={`translate(${xpos},${ypos - 5})`} is enough. Using this will shrink this line a lot.
| ) => { | ||
| const { xpos, ypos, lineName, stationNumber, type_, ...others } = props; | ||
| switch (type_) { | ||
| case 3: //Line end |
There was a problem hiding this comment.
Line end looks super weird on my browser. Will you double-check it?
There was a problem hiding this comment.
It's ok before.....Let me review.
| type_: number; | ||
| } & React.SVGProps<SVGGElement> | ||
| ) => { | ||
| const { xpos, ypos, lineName, stationNumber, type_, ...others } = props; |
There was a problem hiding this comment.
No ...others is needed, isn't it?
#220 Part 1 (#231) and part 2 merge in one pull request.