Skip to content

QF | Line Diagram PDF schedules - #3481

Open
lvachon1 wants to merge 6 commits into
mainfrom
lev/line_diagram/right_rail_PDF
Open

QF | Line Diagram PDF schedules#3481
lvachon1 wants to merge 6 commits into
mainfrom
lev/line_diagram/right_rail_PDF

Conversation

@lvachon1

@lvachon1 lvachon1 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Scope

Asana Ticket: QF | 💈 PDF schedules

Implementation

Added Route PDF Schedules and Maps to the Right Rail of the new Line Diagram

Updated the styles to reflect the new design

Screenshots

Screenshot 2026-09-09 at 1 06 59 PM

How to test

http://localhost:4001/schedules/CR-NewBedford/line_new - (or any route with PDFs) Confirm that the PDF link shows up and goes to the correct PDF

@lvachon1
lvachon1 marked this pull request as ready for review September 9, 2026 15:40
@lvachon1
lvachon1 requested a review from a team as a code owner September 9, 2026 15:40
@lvachon1
lvachon1 requested a review from joshlarson September 9, 2026 15:41
@smartling-github-connector smartling-github-connector Bot added the Not Ready for Translation Remove this when the PR is approved to trigger a Smartling translation job label Sep 9, 2026
@lvachon1
lvachon1 marked this pull request as draft September 9, 2026 16:57
@lvachon1
lvachon1 marked this pull request as ready for review September 9, 2026 19:03

@joshlarson joshlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice! Left two code-style suggestions - take 'em or leave 'em.

Comment on lines +37 to +43
date = Map.get(socket.assigns, :date, @date_time_module.now())

pdfs =
Dotcom.RoutePdfs.fetch_and_choose_pdfs(
route_id,
date
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (non-blocking): You could restructure this whole thing to first assign a date if missing, using assign_new, and then add a separate assign_pdfs function. Like so:

def mount(...) do
  {:ok,
   socket
   |> assign(:route_id, route_id)
   # ... more assigns
   |> assign(:tab_params, tab_params)
   |> assign_new(:date, &@date_time_module.now/0)
   |> assign_pdfs()}
end

# ...

defp assign_pdfs(%{assigns: %{date: date}} = socket) do
  pdfs =
    Dotcom.RoutePdfs.fetch_and_choose_pdfs(
     route_id,
     date
   )

  assign(socket, :route_pdfs, pdfs)
end

A few things to note about ☝️:

  1. assign_new/3 takes a 0-arity function, not a value, hence &@date_time_module.now/0 rather than @date_time_module.now()
  2. Given that assign_pdfs is called after assign_new(:date, ...), you wouldn't need to worry about handling the case where the socket doesn't have a date assigned.

I recognize that this is kind of an aesthetic thing - I find it easier to follow mount/3 when it's defined as a simple list of socket |> assign_this(...) |> do_that(...) |> assign_frobnicator(...). Hence why the comment is non-blocking 🙂

Comment on lines +152 to +155
case Map.get(assigns, :route_pdfs, []) do
{:error, _} -> []
pdfs when is_list(pdfs) -> pdfs
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (non-blocking): Would it make sense to move this to the assigned route_pdfs field, rather than doing this logic in the component?

If you follow the suggestion in my earlier comment about adding an assign_pdfs/1 function, then I think that function could be a good place to transform {:error, _} into [] as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Not Ready for Translation Remove this when the PR is approved to trigger a Smartling translation job

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants