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

[PR] Phoenix 1.7rc upgrade #55

Merged
merged 20 commits into from
Dec 9, 2022
Merged

[PR] Phoenix 1.7rc upgrade #55

merged 20 commits into from
Dec 9, 2022

Conversation

LuchoTurtle
Copy link
Member

closes #54

Still very much in progress. It's taking much longer than expected. The migration guide isn't really helping much, as I'm constantly getting stuck with routing.

The Routes.Helper isn't dynamically created no more and there are no instances of how to fix this in the guide, so I had to look in the docs for anything similar.

I'll keep on working 👍

@LuchoTurtle LuchoTurtle self-assigned this Dec 5, 2022
@LuchoTurtle LuchoTurtle marked this pull request as draft December 5, 2022 18:05
@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Dec 5, 2022

This is super frustrating.
I'm stuck on trying to render a component (new.html.heex) inside index.html.heex. I could simply just use the code inside it but that would go against the grain with the tutorial.

The old way was using <%= render "form.html", Map.put(assigns, :action, Routes.item_path(@conn, :create)) %> but there I can't find any mention of rendering .heex files inside other .heex files (component reusability)
https://hexdocs.pm/phoenix/1.7.0-rc.0/components.html

I really don't want to have to import Phoenix.View as dependency -> https://phoenixframework.org/blog/phoenix-1.7-released

I shouldn't need to... 😤

@LuchoTurtle
Copy link
Member Author

Opened a question yesterday night but it was pending.
Got some answers after it got accepted during the night so I might give these a whirl.

https://elixirforum.com/t/how-to-use-embed-heex-inside-heex-phoenix-1-7rc/52281

(although I think only one is viable)

@LuchoTurtle
Copy link
Member Author

Got it to work. Explained it here -> https://elixirforum.com/t/how-to-use-embed-heex-inside-heex-phoenix-1-7rc/52281/4?u=luchoturtle.

Calling <%= new(Map.put(assigns, :action, ~p"/items/new")) %> works and embeds the form properly.

Let's keep going...

@nelsonic
Copy link
Member

nelsonic commented Dec 6, 2022

Great that @t0t0's answer on Elixir Forum was helpful. 🎉
Really looking forward to seeing the progress on this PR. 👌
It's suuuuuuuper insightful to people considering using Phoenix how much has changed over the past 18 months. 💭

@LuchoTurtle
Copy link
Member Author

Calling actions from the view seems to be the bane of my existence. It's super demotivating when nothing seems to work.
I truly have no idea why they would remove the ability to use Routes.item_path(@conn, :edit, item) to call actions from the template.
I thought <%= delete(Map.put(assigns, :action, ~p"/items/#{item.id}")) %> would work but it is not and it puts into question how the other one worked.
Spending two days on this is blasphemous and embarassing. Granted, the README will be updated accordingly but it's incredible how from 1.5 to 1.7 nothing works...

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Dec 6, 2022

...
And calling actions from the template.

Why does it work here?

<.table id="items" rows={@items} row_click={&JS.navigate(~p"/items/#{&1}")}>
  <:action :let={item}>
    <.link href={~p"/items/#{item}"} method="delete" data-confirm="Are you sure?">
      Delete
    </.link>
  </:action>
</.table>

And not simply?

   <.link
      class="destroy"
      navigate={~p"/items/#{item.id}"}
      method="delete"
    >
    </.link>

And if I want to use <:actions> by itself and try to make it work I get a
Invalid slot entry <:action>. A slot entry must be a direct child of a component. Looking for this error leads me here phoenixframework/phoenix_live_view#1892 but it really isn't all that helpful.

I mean, if I run mix phx.routes, I get the following output:

  GET     /                                      AppWeb.PageController :home
  GET     /items/toggle/:id                      AppWeb.ItemController :toggle
  GET     /items                                 AppWeb.ItemController :index
  GET     /items/:id/edit                        AppWeb.ItemController :edit
  GET     /items/new                             AppWeb.ItemController :new
  GET     /items/:id                             AppWeb.ItemController :show
  POST    /items                                 AppWeb.ItemController :create
  PATCH   /items/:id                             AppWeb.ItemController :update
  PUT     /items/:id                             AppWeb.ItemController :update
  DELETE  /items/:id                             AppWeb.ItemController :delete

Why isn't there a simple way of accessing /items/:id using the delete method? At this point I feel like I am almost forced to create a new route in router just to handle deleting files - but this shouldn't happen. It already works but why do I need to wrap anything in an element like a <.form> or <.table>? I shouldn't need to...

Here's the question I posted on Elixir Forums https://elixirforum.com/t/cant-get-delete-links-to-work-in-phoenix-1-7rc/52317

Sigh

@nelsonic
Copy link
Member

nelsonic commented Dec 7, 2022

Thank you for capturing your frustrations on this quest. 🙏
Keep going, you’re almost there! 🤞

@LuchoTurtle
Copy link
Member Author

Finished updating all the README and all the permalinks in each step.
Haven't changed yet references to Heroku, though.
Will get with deploying to Fly.io and update this readme accordingly. That way I should close two issues with this PR at once.

@LuchoTurtle LuchoTurtle requested a review from nelsonic December 9, 2022 09:39
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Dec 9, 2022
@LuchoTurtle LuchoTurtle marked this pull request as ready for review December 9, 2022 09:39
@LuchoTurtle
Copy link
Member Author

Assigning his over to @nelsonic . Need to get this merged in order to deploy it to Fly.io

@@ -0,0 +1,3 @@
defmodule App.Mailer do
Copy link
Member

Choose a reason for hiding this comment

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

given that we aren't using mailer in this app, could we just delete this file? ✂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want me to delete these files? Why did you merge if you gave this feedback?

@@ -0,0 +1,621 @@
defmodule AppWeb.CoreComponents do
Copy link
Member

Choose a reason for hiding this comment

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

This auto-generated file is massive. 😮
But at least it has good docs. 👍

JS commands may be passed to the `:on_cancel` and `on_confirm` attributes
for the caller to react to each button press, for example:

<.modal id="confirm" on_confirm={JS.push("delete")} on_cancel={JS.navigate(~p"/posts")}>
Copy link
Member

Choose a reason for hiding this comment

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

Modals are horrible in most cases, don't use them! dwyl/product-ux-research#38

)
end

def show_modal(js \\ %JS{}, id) when is_binary(id) do
Copy link
Member

Choose a reason for hiding this comment

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

don't.

@@ -50,13 +50,23 @@ msgid "are still associated with this entry"
msgstr ""

## From Ecto.Changeset.validate_length/3
msgid "should have %{count} item(s)"
Copy link
Member

Choose a reason for hiding this comment

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

are we actually using gettext for translating the UI? 💭
If so, we could add a whole new section to this tutorial! 💡

@@ -50,13 +50,23 @@ msgid "are still associated with this entry"
msgstr ""

## From Ecto.Changeset.validate_length/3
msgid "should have %{count} item(s)"
Copy link
Member

Choose a reason for hiding this comment

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

are we actually using gettext for translating the UI? 💭
If so, we could add a whole new section to this tutorial! 💡

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@LuchoTurtle this is a great update. Nice work! 🎉

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@LuchoTurtle this is a great update. Nice work! 🎉

@nelsonic nelsonic merged commit 5a8e85f into main Dec 9, 2022
@nelsonic nelsonic deleted the phoenix-1.7rc branch December 9, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Chore: Update Phoenix 1.7 Rc ⬆️
2 participants