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

Changed structure of the demo-head and footerHTML #351

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

azeller
Copy link

@azeller azeller commented Aug 9, 2021

Explanation

I think it's a really bad idea to have one chunk with only opening tags and the other one with closing tags that depend on each other. I'd avoid that in any situation. Editors like Ace will always show you if you missed a closing tag, you can expand/collapse those, as long as your HTML is valid and consistent. Having it the way it is now is sloppy and something we've seen in a lot of MODX sites that other people built. If it's in the official documentation, we can't even blame them. I'd recommend changing this for several obvious reasons. The most important reason would be to easily check if your HTML is valid. Having to touch two chunks when adding an opening or a closing tag that is wrapping the entire page is just a bad idea.

I think it's a really bad idea to have one chunk with only opening tags and the other one with closing tags that depend on each other. I'd avoid that in any situation. Editors like Ace will always show you if you missed a closing tag, you can expand/collapse those, as long as your HTML is valid and consistent. Having it the way it is now is sloppy and something we've seen in a lot of MODX sites that other people built. If it's in the official documentation, we can't even blame them. I'd recommend changing this for several obvious reasons. The most important reason would be to easily check if your HTML is valid. Having to touch two chunks when adding an opening or a closing tag that is wrapping the entire page is just a bad idea.
@Mark-H
Copy link
Collaborator

Mark-H commented Aug 10, 2021

I very regularly use that structure where the templates themselves contain very little actual markup so I don't immediately agree with your proposal being better as it'll cause duplication.

Perhaps rather than replacing what's in the docs, offer it up as an alternative approach? There can be more than one way to do things that are all valid with their own pros/cons.

@azeller
Copy link
Author

azeller commented Aug 10, 2021

That was the idea. I think this is a better way though, but everybody can do their own thing of course.

After all, it's a PR that can be denied with no hard feelings whatsoever. Opening up eyes to different or better approaches only works if there's a concrete alternative ;)

However, I can of course leave the original approach and put the other one side by side to it. I just thought it would be confusing to do so

@Mark-H
Copy link
Collaborator

Mark-H commented Aug 12, 2021

I don't want to decline pull requests, ever. ;)

As the rest of the page builds upon the entire head markup being in the headerHtml chunk, it would be quite confusing to accept this as-is. Perhaps leave the chunk code as it is currently, but add a comment block (with >) below it to highlight the flexibility and encouraging people to think about the right level of abstraction for their project?

There are different ways to break up a template into re-usable chunks. For example some people prefer to leave more of the HTML structure (like the doctype, html, head and body tags) in the templates, and to only have their respective contents in chunks. As a general guideline, consider extracting any section or part of a template to a chunk if it will be used exactly the same on a different template.

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.

2 participants