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

Upgrade the folder structure to use assets folder #9

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

9to1url
Copy link
Contributor

@9to1url 9to1url commented Mar 13, 2020

Last time upgrade to phx 1.4, this time also upgrade the folder structure and match phx 1.3/1.4.

Copy link
Owner

@meddle0x53 meddle0x53 left a comment

Choose a reason for hiding this comment

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

Looks good.
I have a few comments. Also this can not be merged before the other PR in the blogit repo is merged and deployed to hex.pm.
Also try running the tests locally - they have to pass (don't have a CI env yet)

@@ -0,0 +1,4 @@
[
import_deps: [:phoenix],
Copy link
Owner

Choose a reason for hiding this comment

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

Good that you introduced the formatter!

config/dev.exs Outdated
@@ -34,5 +34,5 @@ config :logger, :error_log, path: "log/error.log", level: :error
config :phoenix, :stacktrace_depth, 20

config :blogit,
repository_url: "https://github.com/meddle0x53/blogit_sample",
repository_url: "https://github.com/9to1url/blogit_sample.git",
Copy link
Owner

Choose a reason for hiding this comment

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

This should be left as it is?

mix.exs Outdated
deps: deps()]
[
app: :blogit_web,
version: "0.14.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Bump the version to 0.15.0

mix.exs Outdated
{:phoenix_pubsub, "~> 1.0"},
{:phoenix_html, "~> 2.6"},
{:phoenix_live_reload, "~> 1.0", only: :dev},
{:gettext, "~> 0.11"},
{:cowboy, "~> 1.0"},
{:poison, "~> 3.1.0"},
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you need poison if Jason is used now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Poison been reference in endpoint.ex at 2018, if not adding this will not compile. I will remove the Poison at endpoint.ex and replace with Jason.

@9to1url
Copy link
Contributor Author

9to1url commented Mar 13, 2020

I tried the mix test, 10 failured. I also clone your original one and run mix test. it also failured 10. Any idea?

@9to1url
Copy link
Contributor Author

9to1url commented Mar 13, 2020

Hi Meddle,
Besides the mix test, I fixed all the issues for the code review.

@meddle0x53
Copy link
Owner

Sorry it took me sometime to get back to you.
This 1.2.3 version of Blogit is now available!

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