Conversation
Co-Authored-By: Topher Hunt <hunt.topher@gmail.com>
tensiondriven
changed the title
cmcaine
left a comment
cmcaine
left a comment
There was a problem hiding this comment.
Hi! I don't have permission to commit this, but I was thinking of using your branch, so I thought I'd review your code and offer some comments.
Thanks for writing this fork and I hope some of these comments are useful!
|
|
||
| eex = Renderer.precompile(source) |
There was a problem hiding this comment.
| eex = Renderer.precompile(source) | |
| eex = source |> Renderer.precompile() |
Removing a formatting change to simplify the diff.
| defmacro function_from_file(kind, name, file, args \\ [], opts \\ []) do | ||
| quote bind_quoted: binding() do | ||
| require EEx | ||
|
|
There was a problem hiding this comment.
Removing a formatting change to simplify the diff.
|
|
||
| alias Slime.TemplateSyntaxError | ||
|
|
||
| @eex_delimiters {"#" <> "{", "}"} |
There was a problem hiding this comment.
| @eex_delimiters {"#" <> "{", "}"} | |
| @eex_delimiters {"\#{", "}"} |
Any reason not to write this way?
| def eex_delimiters, do: @eex_delimiters | ||
| def heex_delimiters, do: @heex_delimiters | ||
|
|
||
| def compile([], _delimiters), do: "" |
There was a problem hiding this comment.
| def compile([], _delimiters), do: "" | |
| def compile(x), do: compile(x, @eex_delimeters) | |
| def compile([], _delimiters), do: "" |
Might (or might not!) be good to avoid breaking the public API of the module by preserving compile/1.
If this is done, all the changes to test/compiler_test.exs can be reverted.
| raise TemplateSyntaxError, | ||
| line: 0, | ||
| message: "I found a HEEx component, but this is not compiling to a HEEx file", | ||
| line_number: 0, | ||
| column: 0 | ||
| end |
There was a problem hiding this comment.
Would be useful for debugging to print the HEExNode here or tell the user to look for tags that begin with :, I think.
| alias Slime.TemplateSyntaxError | ||
|
|
||
| @eex_delimiters {"#" <> "{", "}"} | ||
| @heex_delimiters {"{", "}"} |
There was a problem hiding this comment.
As far as I can tell, the values of these globals are never used, except for comparing if the passed "delimiters" parameter matches one or the other.
Given this, perhaps it would be better to pass a symbol instead? Something like complile(x, :eex) or compile(x, %{output_format: :eex})?
| end | ||
|
|
||
| @doc """ | ||
| Compile Slime template to valid EEx HTML. |
There was a problem hiding this comment.
| Compile Slime template to valid EEx HTML. | |
| Compile Slime template to valid HEEx HTML. |
| iex> Slime.Renderer.precompile(~s(input.required type="hidden")) | ||
| "<input class=\\"required\\" type=\\"hidden\\">" |
There was a problem hiding this comment.
| iex> Slime.Renderer.precompile(~s(input.required type="hidden")) | |
| "<input class=\\"required\\" type=\\"hidden\\">" | |
| iex> Slime.Renderer.precompile_heex(~s(:component.required type="hidden")) | |
| "<.component class=\\"required\\" type=\\"hidden\\">" |
| def render(slime, bindings \\ [], opts \\ []) do | ||
| slime | ||
| |> precompile | ||
| |> precompile() |
There was a problem hiding this comment.
| |> precompile() | |
| |> precompile |
Removing a formatting change to simplify the diff.
| ``` | ||
|
|
||
| ```html | ||
| <tt>Always bring a towel.<tt> |
There was a problem hiding this comment.
Shouldn't this be either
"<tt><Always>bring a towel.</Always></tt>"
or
tt
| Always bring a towel.
?
There was a problem hiding this comment.
Yeah, you're right. I'll add a suggestion for that, too.
iex(1)> Slime.Renderer.precompile("tt\n Always bring a towel")
"<tt><Always>bring a towel</Always></tt>"
iex(2)> Slime.Renderer.precompile("tt\n | Always bring a towel")
"<tt>Always bring a towel</tt>"
| ```slim | ||
| tt | ||
| Always bring a towel. | ||
| ``` |
There was a problem hiding this comment.
| ```slim | |
| tt | |
| Always bring a towel. | |
| ``` | |
| ```slim | |
| tt Always bring a towel. | |
| ``` |
If you split the tag across multiple lines you have to use |.
|
@cmcaine @tensiondriven apologizes for being absent, some unfortunate events took me away from open source for awhile but I'm getting back into the swing of things. I'd love to move forward with this PR, how can I help? |
4 participants
This PR adds support to the
slimepackage for HEEx. The main changes to the slim/slime grammar are:When rendering HEEx, attribute values will be escapes with
{ }instead of#{ }.When rendering HEEx, tag names prefixed with a colon (
:) will be rendered with a dot (.).The dot and curlybrace syntax changes are specific to HEEx.
This is accomplished by adding a
precompile_heexfunction, similar to the existingprecompilefunction. It works by passing in a set of delimiters for escaping attribute values:These delimiters are passed down into
Slime.Compilerto allow it to return the proper data structures, depending on if HTML or HEEx is requested. Additionally,Slime.Parser.Nodes.HEExNodewas added which is returned inSlime.Parser.Transform.The PEG grammar was updated to add the case where a tag name is prefixed with a colon (
:). When precompile (which targets EEx, not HEEx) is called with input that contains a colon, aSlime.TemplateSyntaxErroris raised.Tests were added in
test/rendering/heex_test.exs. Testing the HEEx functionality end-to-end was difficult because it would add a dependency onphoenix_slime, so those tests were omitted. This was picked up in my fork ofphoenix_slimehere: https://github.com/tensiondriven/phoenix_slime/blob/master/test/phoenix_slime_heex_test.exsI also set up a test Phoenix app which pulls in the new versions of Slime and PhoenixSlime here:
https://github.com/tensiondriven/phoenix_slime_test
I expect there are still some issues hiding in here somewhere, so please don't merge until enough review has been done. For now, I'm submitting the PR to get the review process going. The test app (linked above) is working and contains as many cases as I could think to test, but I'm sure there are more that are lurking.
Co-Authored-By: Topher Hunt hunt.topher@gmail.com