Skip to content

Adding standard layout.html.twig for the TwigBundle recipe #161

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

Merged
merged 6 commits into from
Aug 31, 2017
Merged

Adding standard layout.html.twig for the TwigBundle recipe #161

merged 6 commits into from
Aug 31, 2017

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Aug 28, 2017

Q A
License MIT

I think it makes sense to start with a base.html.twig in the templates directory.

This is an exact copy of the one from the SE... except that the favicon.ico link tag is gone (since we don't give the user this file).

Btw, would there be any issue with making this just base.twig? Is it time & is there a downside to dropping the .html part (other than breaking from tradition)?

symfony-bot
symfony-bot previously approved these changes Aug 28, 2017
symfony-bot
symfony-bot previously approved these changes Aug 28, 2017
@Pierstoval
Copy link
Contributor

Even though Twig is used to generate HTML most of the time, I'm not sure removing the template format from the name is a good idea.

Twig errors templates follow this:
image.

I don't see the value of removing it, unless writing some few characters is a big problem for DX.

@jvasseur
Copy link

Twig is using the extension of the file to choose what needs escaping. It defaults to html if there is no known extension but it's still a good practice to keep the extension.

@weaverryan
Copy link
Member Author

Cool. Let's keep the .html.twig extension, and hopefully get some 👍 to merge this guy as is :)

Selfishly, I have a Flex workshop on Friday... and this would help me remove a manual step 😇

chalasr
chalasr previously approved these changes Aug 29, 2017
@fabpot
Copy link
Member

fabpot commented Aug 29, 2017

base or layout?

symfony-bot
symfony-bot previously approved these changes Aug 31, 2017
@weaverryan weaverryan changed the title Adding standard base.html.twig for the TwigBundle recipe Adding standard layout.html.twig for the TwigBundle recipe Aug 31, 2017
symfony-bot
symfony-bot previously approved these changes Aug 31, 2017
@weaverryan
Copy link
Member Author

Yea, let's use layout.html.twig - layout is the standard word for this. I've also deleted the .gitignore in templates/, since that directory is no longer empty.

This is ready for 👍 !

@chalasr
Copy link
Member

chalasr commented Aug 31, 2017

No strong opinion myself, but this concern has been raised in symfony/symfony-standard#1034 (comment) and it seems that some folks did not agree with layout.

@javiereguiluz
Copy link
Member

I'd prefer base.html.twig because this template just provides a base skeleton to show HTML contents. layout.html.twig would be better for example if we defined some main and sidebar blocks too.

symfony-bot
symfony-bot previously approved these changes Aug 31, 2017
@weaverryan
Copy link
Member Author

Hmm.. indeed - seems there were several 👍 to stay with base.html.twig. Since switching from base.html.twig to layout.html.twig is not a clear win, let's stay with base.html.twig, which has the benefit of not being a change (so nothing new to teach, no docs / blog posts to update, etc)

I've changed back to base.html.twig. Ready for votes once again!

<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8" />
Copy link
Member

Choose a reason for hiding this comment

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

Can we please change this line to:

<meta charset="UTF-8">

Notes:

So, even if it could be valid for BC purposes, it looks like <meta ... /> should be <meta ...> (same for other empty elements).

Choose a reason for hiding this comment

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

The / is valid and mentioned in the spec:
https://www.w3.org/TR/html5/syntax.html#start-tags

If the element is one of the void elements, or if the element is a foreign element, then there may be a single "/" (U+002F) character.

So both versions are totally valid but the / is just an optional character that hold no meaning.

Copy link
Member

Choose a reason for hiding this comment

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

@jvasseur yes, it's valid. But the "cool kids" (W3C, MDN) no longer use it. Symfony Flex and recipes are like a new and refreshing Symfony, so I thought we should go for the cool new way of writing those empty elements 😎

Choose a reason for hiding this comment

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

Yes, I'm for removing it to.

I just wanted to point to the part of the spec that was mentioning the fact that it's optional and doesn't hold any meaning.

Copy link
Contributor

Choose a reason for hiding this comment

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

It holds a meaning: <meta> is HTML whereas <meta /> is XML. That's the slight difference between the two.
Before, we used to write <meta /> when writing xHTML 1.1, and <META> when writing HTML 4.
HTML 5 allows both syntaxes, but as said, "cool kids" write cool code 😎

@weaverryan
Copy link
Member Author

Done! Slash removed.

Can I get some up votes please? 😇 😇 I have a Flex workshop in the morning - having this file in the recipe removes an extra step. So, I don't want to rush things... but if this is ready... let's vote it in!

@symfony-bot symfony-bot merged commit 32a4788 into symfony:master Aug 31, 2017
@weaverryan weaverryan deleted the patch-1 branch August 31, 2017 13:03
@weaverryan
Copy link
Member Author

you guys are the best ;)

VolCh pushed a commit to VolCh/recipes that referenced this pull request Nov 30, 2017
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.

8 participants