Skip to content
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

feat: add scaffolding for shuttle page #1019

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

meagharty
Copy link
Contributor

Summary of changes

Asana Ticket: Generate scaffolding for Create/View/Edit Shuttle Route Definition pages

  • Database tables were created in previous PR
  • This adds the scaffolding for the page that will define shuttles
  • The form in this PR scaffolds the basic shuttle definition (name, disrupted_route_id, status) but the full implementation is covered by another ticket

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

@meagharty meagharty requested review from a team and jzimbel-mbta and removed request for a team October 11, 2024 20:49
<.input field={f[:shuttle_name]} type="text" label="Shuttle name" />
<.input
field={f[:status]}
type="select"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a PR open to fix a bug with the bootstrap styling for select inputs that was added to core_components: #1018

Right now this field has an error but it doesn't display properly to the user (it's hidden because it's not marked properly as is-invalid)

@meagharty meagharty changed the title Add scaffolding for shuttle page feat: add scaffolding for shuttle page Oct 11, 2024
@meagharty meagharty force-pushed the meag/add-scaffolding-for-shuttle-page branch from a62a8f2 to e11a1bd Compare October 16, 2024 15:39
Copy link
Member

@jzimbel-mbta jzimbel-mbta left a comment

Choose a reason for hiding this comment

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

Good to go! Just 2 non-blocking questions.

This was a good demo of heex templates, which I haven't yet used.

@@ -231,4 +231,88 @@ defmodule Arrow.Shuttles do
def change_shape(%Shape{} = shape, attrs \\ %{}) do
Shape.changeset(shape, attrs)
end

alias Arrow.Shuttles.Shuttle
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for putting the alias here rather than at the top of the module? Is it sort of to separate the shape-related functions above from the shuttle-related functions below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phoenix code generators automatically added this to the end of the file. I was thought of it as separation from the shape-related functions but I don't feel strongly about it (and I don't know how intentional it is from code gen).

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, I didn't even know this code was generated! Will take another look at Phoenix's codegen docs 👀


@spec create_shuttle(
:invalid
| %{optional(:__struct__) => none(), optional(atom() | binary()) => any()}
Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen the none type use like this before—does this essentially say that if you pass a map to create_shuttle/1, it has to be a bare map and not a struct? i.e., it's not allowed to have a :__struct__ key?

(I thought none / no_return was only useful for indicating that a function never returns, due to raising, throwing, terminating the process etc; so I'm not sure of its semantics in a typespec.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, I'll remove, I didn't notice this.

This annotation was created by VsCode so I can't comment on why it generated the annotation like this. I must have accidentally added it at some point while viewing the file.

Copy link
Member

Choose a reason for hiding this comment

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

Oh gotcha. I sometimes accidentally click those suggested typespecs and they're almost always totally crazy, especially when any kind of string argument / return is involved!

@meagharty meagharty force-pushed the meag/add-scaffolding-for-shuttle-page branch from e11a1bd to 36040b5 Compare October 21, 2024 20:43
@meagharty meagharty force-pushed the meag/add-scaffolding-for-shuttle-page branch from 36040b5 to 7203404 Compare October 21, 2024 20:51
@meagharty meagharty merged commit 7cd3c7d into master Oct 22, 2024
10 checks passed
@meagharty meagharty deleted the meag/add-scaffolding-for-shuttle-page branch October 22, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants