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

Document LiveView controller example requiring Phoenix controller #1672

Merged
merged 3 commits into from Oct 14, 2021
Merged

Document LiveView controller example requiring Phoenix controller #1672

merged 3 commits into from Oct 14, 2021

Conversation

brettinternet
Copy link
Contributor

It was not apparent to me that the Phoenix.LiveView.Controller required the Phoenix.Controller. If you find this minor edit acceptable, it could help someone else.

Thank you for sharing this incredible project.

@@ -19,7 +19,7 @@ defmodule Phoenix.LiveView.Controller do
## Examples

defmodule ThermostatController do
...
use AppWeb, :controller
Copy link
Member

Choose a reason for hiding this comment

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

I believe the convention is to call those things MyApp.

Suggested change
use AppWeb, :controller
use MyAppWeb, :controller

@josevalim
Copy link
Member

Good idea! I have added some suggestions to provide more context to users.

@brettinternet
Copy link
Contributor Author

Thanks for the suggestions! I found a couple other places where App was used in the docs, so I updated them to MyApp. I'm happy to respond to any other suggestions.

Thanks, Jose, I love your work.

@josevalim josevalim merged commit afdee17 into phoenixframework:master Oct 14, 2021
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@brettinternet brettinternet deleted the docs/controller-dep branch October 14, 2021 16:12
Zurga pushed a commit to Zurga/phoenix_live_view that referenced this pull request Apr 21, 2023
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.

None yet

2 participants