-
Notifications
You must be signed in to change notification settings - Fork 1k
Add MacroComponent directives and Root Tag Attribute functionality #4116
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
Changes from 1 commit
a720189
0d176ce
249e76d
1fc284c
932c135
f458b8d
80bc689
1bf4cce
7ccbb3d
fa63849
f40e6ae
150036a
b7232af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,30 @@ defmodule Phoenix.Component.MacroComponent do | |
| # LiveView's end to end tests: a macro component that performs | ||
| # [syntax highlighting at compile time](https://github.com/phoenixframework/phoenix_live_view/blob/38851d943f3280c5982d75679291dccb8c442534/test/e2e/support/colocated_live.ex#L4-L35) | ||
| # using the [Makeup](https://hexdocs.pm/makeup/Makeup.html) library. | ||
| # | ||
| # ## Directives | ||
| # | ||
| # Macro components may return directives from `transform/2` which can be used to influence | ||
| # other elements in the template outside of the macro component at compile-time. For example: | ||
| # | ||
| # ```elixir | ||
| # defmodule MyAppWeb.TagRootSampleComponent do | ||
| # @behaviour Phoenix.Component.MacroComponent | ||
| # | ||
| # @impl true | ||
| # def transform(_ast, _meta) do | ||
| # {:ok, "", %{}, [root_tag_annotation: "test1", root_tag_annotation: "test2"]} | ||
| # end | ||
| # end | ||
| # ``` | ||
| # The following directives are currently supported: | ||
| # | ||
| # ### Options | ||
| # | ||
| # * `:root_tag_annotation` - A value to apply as an annotation to all root tags during template compilation. | ||
| # Requires that a `:root_tag_annotation` be configured for the application. Value must be a string. May be | ||
| # provided multiple times to apply multiple annotations. See the docs for `Phoenix.Component` for details on | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an issue with this: having multiple HTML attributes with the same name is invalid <div phx-r phx-r="foo" phx-r="bar" />@josevalim had an idea to call it # default: no root_tag_attributes configured
config :phoenix_live_view, root_tag_attributes: [some_attribute: "some-value"]Then, the configuration for config :phoenix_live_view, :colocated_css, style_attribute: "phx-r"And we'd add our style attribute to that list: attrs = Application.get_env(:phoenix_live_view, :root_tag_attributes, [])
if style = Keyword.get(Application.get_env(:phoenix_live_view, :colocated_css, []), :style_attribute) do
[{style, true} | attrs]
else
attrs
end
Application.put_env(:phoenix_live_view, :root_tag_attributes, attrs)I'm just not sure right now where that code would live. Then the macro component would return style_attribute = Application.get_env(:phoenix_live_view, :colocated_css, []) |> Keyword.get(:style_attribute, "phx-r")
{:ok, ..., root_tag_attributes: [{style_attribute, hash}]}So we expect different macro components to use different attributes. The tag engine would then merge the attributes from the directive with the attributes from the config (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about something like this? config :phoenix_live_view, root_tag_attribute: "phx-r"config :phoenix_live_view, :colocated_css, style_attribute: "phx-css"style_attribute = Application.get_env(:phoenix_live_view, :colocated_css, []) |> Keyword.get(:style_attribute, "phx-r")
{:ok, ..., root_tag_attributes: [{style_attribute, hash}]}This would mean that all root elements get attrs = Application.get_env(:phoenix_live_view, :root_tag_attributes, [])
if style = Keyword.get(Application.get_env(:phoenix_live_view, :colocated_css, []), :style_attribute) do
[{style, true} | attrs]
else
attrs
end
Application.put_env(:phoenix_live_view, :root_tag_attributes, attrs)ColocatedCSS could then set a lower bound as any element which has Maybe this provides a good middle-ground. Thoughts?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That works for me! (cc @josevalim)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The discussion @SteffenDE and I had were more from the point of view of adding feature feature. I think for now we only need: config :phoenix_live_view, :colocated_css, style_attribute: "phx-css"And we will initialize if style = Keyword.get(Application.get_env(:phoenix_live_view, :colocated_css, []), :style_attribute) do
[{style, true}]
else
[]
endThe
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. personally, I'd like to avoid css specific code in the tag engine if possible, but José can overrule me :D
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SteffenDE unfortunately you may need to be overulled. Since this is happening at compile-time, I don't think we'd have an appropriate moment to copy the configuration from CSS to root_tag_attributes :( As there is no entry point. I don't think we even require the Phoenix LiveView to be started? Unless we can do it when we start the LivView compiler or similar...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With my proposed approach I think you don't have to copy over anything from CSS to root_tag_attributes (but I may be missing something): this sets what attribute you want to be applied to all roots in all templates, or disable root attribute application entirely (which would mean you couldn't use ColocatedCSS, but your templates wouldn't be polluted by the extra attribute you don't use), and is used in the TagEngine: config :phoenix_live_view, root_tag_attribute: "phx-r"this configures what special attribute to additional apply for ColocatedCSS separately, and is only used in the ColocatedCSS config :phoenix_live_view, :colocated_css, style_attribute: "phx-css"then the TagEngine knows to always apply So the config is completely separate, and we know to inject the CSS specific attributes as we compile that specific template. The WDYT? @josevalim
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are conceptually correct and that's what we want. The downside of this approach is that it configures it twice. It would be nice if the users could opt-in by only configuring the style_attribute. But we can iron out this detail later. So we may need to overrule @SteffenDE for user convenience needs. :) but we can do it later!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that makes sense. If user convenience is the goal, perhaps we could just default the root attribute to being applied as config :phoenix_live_view, root_tag_attribute: false🤔 🤔 🤔 but you are right, we can iron out the details later 😎 👍 |
||
| # root tag annotations and how to configure them. | ||
|
|
||
| @type tag :: binary() | ||
| @type attribute :: {binary(), Macro.t()} | ||
|
|
@@ -128,9 +152,13 @@ defmodule Phoenix.Component.MacroComponent do | |
| @type tag_meta :: %{closing: :self | :void} | ||
| @type heex_ast :: {tag(), attributes(), children(), tag_meta()} | binary() | ||
| @type transform_meta :: %{env: Macro.Env.t()} | ||
| @type directive :: {:root_tag_annotation, String.t()} | ||
| @type directives :: [directive] | ||
|
|
||
| @callback transform(heex_ast :: heex_ast(), meta :: transform_meta()) :: | ||
| {:ok, heex_ast()} | {:ok, heex_ast(), data :: term()} | ||
| {:ok, heex_ast()} | ||
| | {:ok, heex_ast(), data :: term()} | ||
| | {:ok, heex_ast(), data :: term(), directives :: directives()} | ||
|
|
||
| @doc """ | ||
| Returns the stored data from macro components that returned `{:ok, ast, data}`. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a more advanced topic, I'm not sure if we should document it in
Phoenix.Component. Probably better in the macro component docs. Those are private right now, but I think that's fine. The main way users would interact with this would be through colocated CSS for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point :) I was 50/50 on it when I was writing the docs as well. I ended up throwing it in in case they could be useful for some people as CSS or JS selectors, but I think it makes total sense since it is niche functionality to not immediately throw it at people in the Phoenix.Component docs!