Add AshAdmin.Layouts and Phoenix.VerifiedRoutes #68
Conversation
esdrasedu
force-pushed
the
add_layouts
branch
2 times, most recently
from
573eb6d
to
47cb2a1
Compare
|
|
||
| def asset_hash("/statics/" <> path) do | ||
| file = | ||
| Application.app_dir(:ash_admin, ["priv", "static", path]) |
There was a problem hiding this comment.
This seems strange. I've not seen something like this in a phoenix application before, how is it normally done for static assets?
There was a problem hiding this comment.
what exacly it is stange? the way I am reading the asset or reading the assets to get the md5 hash to append to the url?
Let me give you more context, phoenix application normally manages the asset and cache running the task mix phx.digest, the result for that is an asset minified and zipped with this pattern name asset-#{hash}.#{ext}.gz
But because ash admin is not an application, its assets are not handle for mix phx.digest, we have to do it runtime or manual (digest each asset before release any version)
There was a problem hiding this comment.
I think what feels strange to me is that we're reading a file to hash it to figure out where its minified version is, but we're doing it every time a page load happens. I think it would be better to make this a macro or use a module attribute to store the hash of it at compile time. You can use @external_dependency to cause external files to trigger recompiles.
| @@ -0,0 +1,41 @@ | |||
| <.live_component | |||
There was a problem hiding this comment.
Personally, I prefer having the html just live in the component's render/1, I don't like having to jump back and forth.
There was a problem hiding this comment.
Fair enough, moved back to ex file
lib/ash_admin/router.ex
Outdated
| ) | ||
| opts = | ||
| if Macro.quoted_literal?(opts) do | ||
| Macro.prewalk(opts, &expand_alias(&1, __CALLER__)) |
There was a problem hiding this comment.
There is Macro.expand_literal IIRC. Why are we doing this?
There was a problem hiding this comment.
I think we don't need this anymore, I was using the options to be able to forward the endpoint together, but this is no longer necessary, removed
| @@ -16,7 +16,7 @@ defmodule AshAdmin.Test.PageLiveTest do | |||
| assert html =~ "body" | |||
| assert html =~ "String" | |||
|
|
|||
| {:ok, _view, html} = live(conn, "/api/admin/test") | |||
| {:ok, _view, html} = live(conn, "/api/admin?&resource=test") | |||
|
|
|||
There was a problem hiding this comment.
why did the routes change? Also, this should probably be ?resource=test not ?&resource=test right?
There was a problem hiding this comment.
sorry, it is a typo, fixed
esdrasedu
force-pushed
the
add_layouts
branch
5 times, most recently
from
da600e1
to
5dc536e
Compare
This PR contains couple changes, but everything is related to Phoenix 1.7 migration:
AshAdmin.Layouts, using heex engine and changing they way assets are provided (using Plug.Static)@prefixforash_admin_path, that way we can have a friendly URL pattern matchI haven't replace all
@prefixyet because I want to change the routes too, maybe try something like:api/:resource/:actionbut I still have to check the others components to know if we should have more params