-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: Pre-fare polish -- Text only resizes for bypassed stations #1946
fix: Pre-fare polish -- Text only resizes for bypassed stations #1946
Conversation
Coverage of commit
|
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.
Looks good! Have a couple of questions about consolidating some hardcoded values. Not blocking though so I'll approve now.
<RemedySection | ||
effect={effect} | ||
remedy={remedy} | ||
contentTextSize="large" |
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.
Since we know what size each remedy will use, can we remove this prop and just delegate the hardcoding to the component? Would centralize the different hardcoded values into a single component.
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.
We do need a way of passing the text size one way or another. StandardLayout uses font size large for the remedy, and DownstreamLayout uses font size medium for this component, so the component alone can't dictate which font size it uses
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.
Gotcha. Makes sense.
<StandardIssueSection | ||
issue={issue} | ||
location={location} | ||
contentTextSize={effect === "station_closure" ? contentTextSize : "large"} |
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.
Similar question as my other about the remedy. Removing this prop would also mean an effect prop would be needed, so not sure if there's much benefit for this one.
* feat: Pre-Fare alerts 2.0 CandidateGenerator (#1765) * [feature] Pre-fare alert 2.0 frontend left screen (#1787) * feat: Pre-Fare alerts 2.0 serializer (#1785) * feat: Disruption Diagram - Frontend (#1825) * [tweak] Disruption diagram maths (#1830) * In LocationContext, tag stop sequences by their routes (#1832) * tweak: PreFare simulations (#1837) * [feature] Prefare alerts 2.0 audio (#1824) * feat: Disruption Diagram - Backend (#1828) * feat(WIP): Disruption diagram backend. Working for Blue Line and other basic scenarios only atm. * fix: Update location context stop sequences field in TrainCrowdingTest (#1895) * test: D. diagram unit test for RL trunk statio closure with home stop at Ashmont (#1894) * Added padding-top to layout when there is no banner. (#1902) * Cm/govt ctr gl affected pill fix (#1898) * Cm/adjust diagram size positioning (#1896) * Changed label to display on a single line. (#1899) * Tweaked resizer logic so it maxHeight matches the card height on page. (#1905) * Removed bolding of 'to' from arrow labels. (#1908) * Fixed icon display when current stop is affected. (#1903) * feat: Consistent description of disrupted stop ranges throughout pre-fare alerts (#1912) * Adjusted Ashmont/Braintree destination to proper formatting (#1917) * Refactor disruption diagram SVG (#1914) * Log alert ID and screen's home stop when disruption diagram logic fails (#1920) * polish: Recalculate dimensions in hook (#1921) * Added audio column to GL & PreFare in admin table (#1916) * Draw D. diagrams for multi-line alerts when home stop narrows it to 1 line (#1922) * fix: PreFare Polish - Inside shuttle (#1941) * fix: Pre-fare polish - Boundary alerts banner (#1945) * fix: Pre-fare polish -- Text only resizes for bypassed stations (#1946) * fix: Pre-fare polish -- Station closed takeover text wrapping (#1947) * Downstream shuttle endpoint should be a circle (#1948) * Label splitting logic was buggy (#1949) * Prefare format in config is "Ashmont & Braintree" not "Ashmont/Braintree" (#1954) * Sub-header for takeover alert is now regular weight (#1955) * fix: Prefare alerts polish -- Fix GL headsign in various cases (#1957) * fix: Prefare polish -- Use FreeText to prevent station text wrapping in subheaders (#1956) * fix: Prefare polish -- Alerts that break assumptions use the fallback case (#1958) * fix: Prefare polish -- Reorder audio with alerts before subway status (#1962) * fix: Prefare polish -- New abbreviation rule (#1961) * fix: Prefare Alerts polish -- RL headsigns for branch-only alerts (#1967) * fix: Prefare alerts polish -- Screenplay sim sizing / page layout (#1966) * fix: Prefare polish -- "via" diagram labels need abbreviating (#1968) --------- Co-authored-by: Christian Maddox <christian.maddox93@gmail.com> Co-authored-by: Jon Zimbel <jzimbel@mbta.com>
Notion
During implementation, it was assumed that all alerts would need to have font resized, but it seems that really it's only explicitly designed for station closures. All other fonts are static.
Also, it turns out the max height for the text is also defined in the design, and we don't need to do all those funky calculations with bannerHeight, etc.