refactor: disruption page workflow and dataflow#1238
Conversation
4688eaa to
7045309
Compare
|
@cmaddox5 I picked on you here because this diff for this is pretty large, and you showed up on the blame for the pieces I changed the most. If you have suggestions on how I can make this easier for you to review, I'll happily spend time restructuring if you have suggestion on how I can make this easier to review. Because I reorganized the code, the diff makes it look like a larger change than it is. Attempting to summarize the actual changes vs copied code are:
|
|
There were only minor changes to the unit tests, and the integration tests I only had to change to URL, so that helps give me confidence I didn't change any behavior too. |
cmaddox5
left a comment
There was a problem hiding this comment.
Did a first pass and it's looking good. Diff definitely seems to be mostly cut and pasted code but want to take one more pass to make sure I didn't miss anything.
| <%= if Ecto.assoc_loaded?(@disruption.replacement_services) and Enum.any?(@disruption.replacement_services) do %> | ||
| <div | ||
| :for={replacement_service <- @disruption.replacement_services} | ||
| class="container border-2 border-dashed border-secondary border-mb-3 p-3 mb-3" |
There was a problem hiding this comment.
Ah yes Firas asked me about expanding this page to match the table and I didn't retest afterwards. Everything should be using container-fluid now.
| class="overflow-hidden" | ||
| style="display: none" | ||
| phx-mounted={ | ||
| JS.show( |
There was a problem hiding this comment.
I'm a big fan of these transitions!
| <div class="col-lg-3"> | ||
| <.button | ||
| type="button" | ||
| <.link |
There was a problem hiding this comment.
I know you didn't edit the text on any of these, but could we go ahead and change button text to be sentence case as per zeroheight? Then we can just go with sentence case for buttons in the future and be confident we are being consistent.
There was a problem hiding this comment.
I think I've updated everything with sentence case now. Let me know if you spot something I missed.
53cb0e8 to
b294347
Compare
cmaddox5
left a comment
There was a problem hiding this comment.
After another pass, I think this is good to go!

Summary of changes
Asana Ticket: [stub] 🏹💅🏻🇵🇱 Create a separate "create" view for disruptions (so we have sensible CRUD)
This aims to refactor the disruption page to be easier to reason about. The primary goal is to ensure data flows in a single direction from the parent disruption page -> the child components, eliminating the need for
send(self())and reducing the situations. I'm also attempting to minimize the scope of the LiveComponents to ensure they are responsible for as little as possible, using functional components as much as possible.The biggest changes made to accomplish these goals are:
push_patchis being used the old state actually is still available on the socket and we could do something more intelligent.update_params/apply_actionsetting up the appropriate state, which gets passed to the child views.Oh yeah and for funsies I also did the ticket and gave disruptions seperate view/edit pages.
Reviewer Checklist