-
Notifications
You must be signed in to change notification settings - Fork 16
wip(ScheduleFinderLive): a new services picker! #2849
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
base: main
Are you sure you want to change the base?
Conversation
joshlarson
left a comment
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.
Two preliminary code-level comments, plus...
As long as we're filtering out No Schools services, should we add in a way for a trip in the daily schedule to be marked as a school trip? The current behavior shows those trips on the M-Th schedule, with no way to tell that they don't run every Monday thru Thursday.
I imagine that that's in follow-up ticket territory, but just wanted to flag it, in case!
| end | ||
| end | ||
|
|
||
| defp sort_services_by_date(service, start_date) do |
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.
Naming nit: Technically this function doesn't sort a list of services - it's a service/start-date mapper.
| defp sort_groups({group_name, _}) do | ||
| [ | ||
| "Schedules, ends", | ||
| "Schedules, starts", | ||
| "Holiday", | ||
| "Extra" | ||
| ] | ||
| |> Enum.find_index(&String.contains?(group_name, &1)) | ||
| end |
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.
At a glance, this looks like the sort of thing that bit us with the Now: prefix in system status. Specifically, I think this will preclude translating Schedules and starts/ends, since once those strings are translated, String.contains? won't work anymore.
I think the solution is to push string-logic to the very last possible moment before they land in an HTML template, so instead of constructing and then parsing strings in the backend, I would try to have this code construct more elaborate structs, and serialize them as strings in the frontend.
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.
You're totally right!
Summary of changes
Asana Ticket: Service selector: show dropdown to toggle between types of schedules
Most of these features are in parity with our current schedule finder:
<optgroup>) are edited for clarity, including dates when helpfulI also attempted to add some quality of life improvements around clarifying things like planned work, or extra service. Look at a commuter rail or Green Line for more on that.
I ended up building off of
DotcomWeb.ScheduleController.LineController.services/2(which powers the current Schedule Finder), though I ended up discarding its calculation ofdefault_service?because the one I wrote (captured in:is_now?and:next_available?properties) works better (especially for cases with modified service). Ha!Still to come