-
Notifications
You must be signed in to change notification settings - Fork 12
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
When we have predictions show those over alerts #2120
Conversation
@@ -57,16 +48,23 @@ const DeparturesWithBadge = ({ | |||
if (!(priorityBadge || secondaryBadge)) return <>{children}</>; | |||
const displayBadge = (priorityBadge || secondaryBadge)!; | |||
|
|||
// if the priority badge is present and there is only one departure, show the badge | |||
// this is because we check for predictions and return a single time list if there are none |
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.
A single time list is also always returned if there are commuter rail predictions (as it's designed to only display one time at time).
So. I'm not sure that tying this PR's logic to "time list happens to have length of 1" makes all that much sense.
{priorityBadge ? ( | ||
<div className="font-helvetica-neue fs-14" style={{ float: "right" }}> | ||
See alternatives | ||
{priorityBadge && timeListLength === 1 ? ( |
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.
There's some logic involved in the generation of priorityBadge
that attempts to weight the number of predictions available against the particular type of alerts – have you looked at isSuppressiveAlert
to see whether modifying that would better fit our use case?
@thecristen can you spell out better logic to implement? There are a lot of assumptions and implicit things encoded in this code and, frankly, I'm not sure what fixes/breaks anything. |
Also, since I'm picking up the ticket to add direction id to the alerts, I'm not sure if it is even worth merging this PR since it will just be reversed in the next few days. |
https://app.asana.com/0/555089885850811/1207562164283201/f
This will work in most cases, but there is one case where it will present incorrect information. If there are one or fewer predictions and there is an alert then it will say there is no service. I can see this happening at the end of service for the day. But, it is still better than it saying there is no service for the entire day.