Fixed some coordinates for suspensions. #1907
Conversation
cmaddox5
requested review from
a team and
hannahpurcell
and removed request for
a team
Coverage of commit
|
|
Could you capture the problem in this ticket? Documentation of the bug that needs fixing |
Updated the description to include a ticket I made for this issue. |
Coverage of commit
|
There was a problem hiding this comment.
This is the first disruption diagram frontend PR I've looked at in a long time, and I'm feeling like it's very easy to miss something that only occurs in some cases. Which is why the bug for this PR came up. The file seems brittle with all the math that seems arbitrary instead of programmatically calculated.
Also, all the instance of the same SVG can use the same component, right? Just with different dimension values (width and height are the same) and color. I know we had that "svg bundling" task that didn't go anywhere, but I do think combining multiple definitions for the same svg shape in this file would help. For example, the "x, stop suspended" icon would work for the effect emphasis "x", the SuspensionStopIcon, and StationClosureStopIcon, right? The component could accept with and color props.
Maybe this suggests there should be a separate task to "simplify disruption_diagram.tsx"?
| @@ -196,9 +196,9 @@ const SuspensionStopIcon: ComponentType<IconProps> = ({ x }) => { | |||
|
|
|||
| return ( | |||
| <> | |||
| <rect x={x - iconWidth / 4} width="18" height="16" fill="white" /> | |||
| <rect x={x - iconWidth / 4} y="5" width="18" height="16" fill="white" /> | |||
There was a problem hiding this comment.
Hmmm, it's like zero-ing out the vertical translation didn't change all that much, because all the y's have to be vertically hardcoded anyway if they're not the same height as the line diagram...
One small way this icon can be cleaned up is by putting the translation above both the rect and the path, since they both need to be translated the same amount:
return (
<g transform={`translate(${x - iconWidth / 2}) -2`}>
<rect x="8" y="8" width="18" height="18" fill="white" />
<path
fillRule="evenodd"
clipRule="evenodd"
d="M8.93886 0C8.76494 0 8.5985 0.0707868 8.47786 0.196069L0.178995 8.81412C0.0641567 8.93338 0 9.09249 0 9.25805V21.0682C0 21.238 0.0674284 21.4008 0.187452 21.5208L8.47922 29.8125C8.59924 29.9326 8.76202 30 8.93176 30H21.0611C21.2351 30 21.4015 29.9292 21.5221 29.8039L29.821 21.1859C29.9358 21.0666 30 20.9075 30 20.7419V8.93176C30 8.76202 29.9326 8.59924 29.8125 8.47922L21.5208 0.187452C21.4008 0.0674284 21.238 0 21.0682 0H8.93886ZM7.5935 10.0066C7.34658 10.2576 7.34866 10.6608 7.59816 10.9091L11.957 15.248L7.59623 19.6793C7.34824 19.9313 7.35156 20.3366 7.60365 20.5845L9.73397 22.6794C9.98593 22.9272 10.391 22.9239 10.6389 22.672L15 18.2404L19.3611 22.672C19.609 22.9239 20.0141 22.9272 20.266 22.6794L22.3964 20.5845C22.6484 20.3366 22.6518 19.9313 22.4038 19.6793L18.043 15.248L22.4018 10.9091C22.6513 10.6608 22.6534 10.2576 22.4065 10.0066L20.2613 7.82685C20.0124 7.5739 19.6052 7.5718 19.3537 7.82217L15 12.1559L10.6463 7.82217C10.3948 7.5718 9.98758 7.5739 9.73865 7.82685L7.5935 10.0066Z"
fill="#171F26"
/>
</g>
);
|
@hannahpurcell I tried my best to comb through all of the random numbers in the file and put them in variables so we can have more clarity as to why it is set to what it is. There are still some |
Coverage of commit
|
I'm taking a stab at this in a fork of this branch, fyi |
|
I ended up finding more issues with scaling and positioning. Going to close this and open a new PR that describes everything that is being changed. |
Notion task: [diagram, frontend] Fix positioning of suspension emphasis
This PR is a replacement of #1907. See that PR for previous comments.
When fixing #1896, I tunneled too much on shuttles and forgot to check how the SVG changes affected suspensions. These new Y values put the elements back where they should be.