-
Notifications
You must be signed in to change notification settings - Fork 17
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
V4.1/Accessibility alerts #578
Conversation
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.
So firstly, this is awesome. Thanks for working on this so quickly. We clearly can deliver some real value (and pressure) in v4 with this.
I do have some thoughts on the way we display this data. It's a little busy on lines like the red line. I think this might work better somehow placed on the Trips page and displaying info for the two stations selected AND/OR displaying some kind of aggregate on the Today page. Maybe combining alerts into one and saying
Elevators out of service at: JFK/UMASS, Park Street & South Station
Or
40/45 elevators in service across line
At a minimum, I think we can remove the Upcoming
part of the widget since I don't think they generally plan elevator downtime far in advance, and if they are planning, it's likely for repairs, and we don't need to be critical of the T for doing the right thing
{canShowSlowZonesMap && <Speed />} | ||
<Alerts title="Accessibility" alerts={accessibilityAlerts} /> |
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.
I think if we're going to reuse alerts here, we should add some kind of prop that get's rid of the today
and upcoming
sections, because we probably only care about current accessibility alerts
Looks great. Sorry for the delay, we were in full "finish v4" mode. We ended up reworking this page quite a bit as you may have seen. I think we should get a group together and have a discussion about how we want to portray accessibility data. We could have a design session to talk about some ideas! |
@kmoses228 Now that we've launched v4, it would be a good idea to readdress this and find a good spot for it |
Screen.Recording.2023-08-27.at.4.46.49.AM.mov |
common/api/alerts.ts
Outdated
): Promise<AlertsResponse[]> => { | ||
const url = new URL(`${APP_DATA_BASE_PATH}/api/alerts`, window.location.origin); | ||
|
||
const lineStations = stations[route].stations; |
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 could consider factoring this into a utility like getStationsForLine()
that performs this check.
stationKeys = lineStations.map((station: Station) => station.station); | ||
} | ||
|
||
const options = { ...accessibilityAlertsAPIConfig, stop: stationKeys.join(',') }; |
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.
Also not a comment specific to this PR but I think we ought to have a utility function to handle this request setup for us, as we (myself included) have been duplicating it across these fns.
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.
Added!
common/api/facilities.ts
Outdated
export const fetchAllElevatorsAndEscalators = async ( | ||
line: LineShort | ||
): Promise<FacilitiesResponse> => { | ||
const url = new URL(`${APP_DATA_BASE_PATH}/api/facilities`, window.location.origin); |
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.
Same here (@ALL) we could consider an apiFetch()
helper to handle the base path, params, etc.
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.
Added!
common/types/facilities.ts
Outdated
} | ||
|
||
export interface Facility { | ||
type: string; |
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.
Is there a known set of possible values here?
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.
It seems like it can only be facility
, I'll change this.
@@ -20,6 +20,10 @@ export interface Station { | |||
short?: string; | |||
} | |||
|
|||
export const isLineMap = (obj: LineMap | Station[]): obj is LineMap => { |
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.
Another suggestion for code structure over time (no need to address here): I think we could find a better name for the LineMap
type, especially since we now have more map-related code elsehwere in the codebase. Maybe RouteDescription
?
} | ||
|
||
export const AccessibilityAlert: React.FC<DelayAlertProps> = ({ alert, type, lineShort }) => { | ||
const lineStations = stations[lineShort].stations; |
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.
Here's another place where getStationsForLine()
would be nifty.
> | ||
<span> | ||
{alert.type === AlertEffect.ESCALATOR_CLOSURE | ||
? 'Escalator out of service' |
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.
Consider factoring out the inner string calculation for clarity.
modules/commute/alerts/AlertBox.tsx
Outdated
{relevantAlerts.map((alert: FormattedAlert) => | ||
getAlertComponent(alert, lineShort, type, busRoute) | ||
)} | ||
{relevantAlerts |
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.
Could we move the calculation of this array into a useMemo()
? a JSON.stringify()
is a somewhat expensive operation to do on every render.
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.
Not even really sure why this is here, I'll just remove it
modules/commute/alerts/Time.tsx
Outdated
@@ -70,3 +70,13 @@ export const UpcomingTime: React.FC<TimeProps> = ({ times }) => { | |||
</> | |||
); | |||
}; | |||
|
|||
export const SinceTime: React.FC<TimeProps> = ({ times }) => { |
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.
I think either name is fine but I wonder if we should use TimeSince
to match the showTimeSince
prop elsewhere.
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.
Changing to EffectiveTime
modules/dashboard/Overview.tsx
Outdated
return ( | ||
<PageWrapper pageTitle={'Overview'}> | ||
<div className="grid w-full grid-cols-1 gap-4 md:gap-8 xl:grid-cols-2"> | ||
{tab === 'Subway' && lineShort && <AlertsWidget lineShort={lineShort} />} |
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.
❗️ I think this should go at the bottom. It's good to have this on the overview page, but I don't think we want it to push the graphs down below the fold on mobile.
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.
This looks great to me and I think we should land it as soon as possible! (Sorry for letting it languish a bit).
I do think we probably want to put the alerts below the graphs on the Line Overview page. Thoughts, all?
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.
This looks great to me and I think we should land it as soon as possible! (Sorry for letting it languish a bit).
I do think we probably want to put the alerts below the graphs on the Line Overview page. Thoughts, all?
Looks great, I'm excited to get this in. Thanks Austin and Kevin. I have a few minor suggestions:
|
I'm with @PatrickCleary in thinking we don't need a separate Alerts page right now. I think we could make something like this but for now I think these living exclusively on the overview page is fine by me |
Motivation
Related to #562
We are not currently showing any data related to accessibility on the dashboard, however this is essential for riders who, for example, cannot use stairs. We also want to hold the MBTA accountable for fixing these issues in a timely manner and keeping accessible features in good working order.
Changes
This PR adds accessibility alerts to the "Today" dashboard for each subway line. Users can click on the alert to see the full description and workaround, if available.
I've also added an
/api/facilities
endpoint for fetching all elevators and escalators on a line. This is not in use yet but will be in the future.The next PR will focus on adding a historical dashboard for elevator and escalator downtime per-line.
Testing Instructions
Run the branch locally and interact with the alerts on each line. Also soliciting feedback on the design.