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

docs: specify templates should be on their own line #693

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Waldeedle
Copy link

image

It wasn't explicitly written anywhere that I cannot use a template inline, the LSP is still highlighting it as a component.
Open to discuss whether or not that is accurate because I could see either way being correct. I think another appropriate solution here is to force it to go to the next line if a matching component is found but my only concern there is with name collisions
image

@Waldeedle
Copy link
Author

image
well I guess you could use string literals if you ever truly wanted to have some string with @ and that by default templ should assume its a component and the LSP should push it to the next line 🤷

@joerdav
Copy link
Collaborator

joerdav commented Apr 22, 2024

Thanks for taking the time to document this, I think we should supplement this with an update to the vscode syntax to avoid confusion.

I also think this documentation section could benefit from the input/output style we have in other areas such as: https://templ.guide/syntax-and-usage/css-style-management#html-class-attribute

Just a single input and output should be fine:

templ showLeftWithLabel() {
	<div>
 		Left:
 		@left()
	</div>
	<div>
 		Left: @left()
	</div>
}
templ left() {
	<div>Left</div>
}
<div>
 	Left:
 	<div>Left</div>
</div>
<div>
 	Left: @left()
</div>

@a-h
Copy link
Owner

a-h commented Apr 22, 2024

Me should also fix the underlying issue. It's a bit weird to not be able to do that, right?

@joerdav
Copy link
Collaborator

joerdav commented Apr 22, 2024

That's what I was thinking, I think we could document current behavior and open an issue. I think there's a bit of nuance in supporting template elements mid line.

templ showLeftWithLabel() {
	@left()
	Left: @left()
	[email protected]
}

@Waldeedle
Copy link
Author

Definitely has some nuances. What would the fix look like in your opinions? The LSP pushing the component to its own line? Or that templ can detect that the component exists and generate it? Only reason I didn't opt for fixing it was that I'm unsure whether the behavior would be not intuitive for someone who puts text that's just so happens to match a component function (and they want the text).

@Waldeedle
Copy link
Author

@joerdav I'm not entirely clear on what you meant with your first comment:

I think we should supplement this with an update to the vscode syntax to avoid confusion.

are you suggesting additional pages to be modified and only having the templ input with the html output for the sections?

@joerdav
Copy link
Collaborator

joerdav commented Apr 23, 2024

Sorry for the confusion, what I meant was that if we are going to document that template elements on the same line do not work, then we should change the syntax highlighting to not highlight the element in those cases.

But it sounds like we might be going for a fix instead?

I think what we want is for the following to be valid, and to render left:

templ showLeftWithLabel() {
	Left: @left()
}

But we want to avoid breaking other peoples code who may have @ in their templates already. That may not be possible in cases such as:

templ username() {
    Hello @joerdav 
}

@Waldeedle
Copy link
Author

Waldeedle commented Apr 24, 2024

@joerdav I've implemented what I believe to be a valid fix. There are three cases that I have used to validate it:

  1. @SomeComponent() - since the parentheses are present this is straight forward to parse as a templ element. I assume all elements of this form are correct and just parse them. (Existing behavior of the templ element parser)
  2. @SomeComponent - This requires special handling, I check for whether or not the same text exists in the template as a passed in argument i.e. @SomeComponent templ.Component. This is a very naive approach and I will look at other alternative methods but for now that is parsing correctly
  3. @SomeComponent - without the presence of a passed in variable, it is considered as text.

The only problem is now there are two failing tests. I can definitely fix them to use the new logic from point 2, but I just want to ensure that is the solution we want to proceed with?

	name:  "templelement: bare variables are read until the end of the token",
	input: `@template</div>`,
	expected: TemplElementExpression{
		Expression: Expression{
			Value: `template`,

and

	name: "templelement: simple, block with templelement as child",
	input: `@Other(p.Test) {
		@other2
	}`,
	expected: TemplElementExpression{
		Expression: Expression{
			Value: "Other(p.Test)",

@Waldeedle
Copy link
Author

main...Waldeedle:templ:fix/parse-templ-element-expressions
you can see my current fix in my forked repo, it is under the branch fix/parse-templ-element-expressions

@Waldeedle
Copy link
Author

here's a visual of what I'm referring to
case 1:

templ Repro() {
	<div>
		something: @SomeComponent() // component
	</div>
}

case 2:

templ Repro(SomeComponent templ.Component) {
	<div>
		something: @SomeComponent // component
	</div>
}

Note: currently it checks the whole template string for the SomeComponent templ.Component instead of the current function scope.

case 3:

templ Repro() {
	<div>
		something: @SomeComponent // text
	</div>
}

@Waldeedle
Copy link
Author

@joerdav @a-h when you folks get a second, let me know what you think about the solution 🎉

@Waldeedle
Copy link
Author

@joerdav @a-h gentle reminder, I'm just waiting for feedback before implementing the logic we all agree on here 🙇

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.

3 participants