-
Notifications
You must be signed in to change notification settings - Fork 281
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
feat: Dom Helpers for more concise block code #210
Conversation
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
@@ -17,5 +17,6 @@ module.exports = { | |||
'import/extensions': ['error', { | |||
js: 'always', | |||
}], | |||
'function-paren-newline': 'off', |
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.
this is so you can keep the attribute object on the same line as the function call, to make visually similar with the dom.
p({ class: 'button-container' },
'Paragraph Text'
);
Adding a 👍 on this. Using these DOM helpers highly increased the readability and development speed of our project. It reduces the complexity of the blocks and improves our capacity to understand the final structure just by looking at the JS. |
👍 - @solaris007 and I are working on something similar (arguably not as sophisticated as this proposal) in [0]. I think it would make some sense to have a standard way to create nested Dom structures in a readable way. |
@andreituicu I'm curious what limitations in template literals made you come up with this approach? One benefit I can see is it's easier to attach event listeners and maybe less need for innerHTML, but besides that I'm not sure I see what else you get. I would 100% agree that it's much easier to read and maintain than imperative DOM code. But on the flip side I think the same can be done with string template literals (and still have it be concise. composable and easy to visualize). function renderItem(item) {
return /* html */`
<div class='blog-carousel-item'>
<a href=${item.path}>
<div class='blog-carousel-thumb'>
${createOptimizedPicture(item.image, item.title, 'lazy', [{ width: '800' }])}
</div>
<div class='blog-carousel-caption'>
<h3>${item.title}</h3>
<p class='blog-description'>${item.description}</p>
<p class='button-container'>
<a href=${item.path} aria-label='Read More' class='button primary'>Read More</a>
</p>
</div>
</a>
</div>
`;
} |
@dylandepass, one problem we had is that as soon as you have user (i.e., author) input to put into the template, it requires escaping (to prevent mistakes and XSS etc.). At that point, the template for innerHtml doesn't work anymore. |
@dylandepass besides the point that @karlpauls makes about XSS, the snippet you shared will unfortunately not generate the expected DOM. The resulting string will be: <div class='blog-carousel-item'>
<a href=/path/to/resource>
<div class='blog-carousel-thumb'>
[object HTMLPictureElement]
</div>
<div class='blog-carousel-caption'>
<h3>Title</h3>
<p class='blog-description'>Description ...</p>
<p class='button-container'>
<a href=/path/to/resource aria-label='Read More' class='button primary'>Read More</a>
</p>
</div>
</a>
</div> which when you try to set it as the There are other minor things that I like about the approach from this PR, which could be argued are subjective/a matter of taste:
And just say, I'm not trying to suggest that people should not use string literals. I think each approach has it's place in the code and one does not exclude the other. Personally, I liked this one better and the fact that there were other projects who took the time to create similar implementations suggested that there might be a pattern. |
@karlpauls Understood, and that makes sense. For the eecol project we ran our template strings through DOMPurify where necessary. You'll take a bit of a hit but should still be possible to get to 100 on PSI. @andreituicu Correct, my bad, I through the example together quickly without thinking too much about it. I solved this in the past by using
One note, you can fix the syntax highlighting and code completion issue in vscode with this extension.
For sure, thanks for clarifying the gaps. I just wanted to better understand the pain points here and see if there was something else I was missing. |
To me, this looks like a templating language and templating languages should be a user choice, not a framework default. The most vanilla thing that I could imagine is based on the HTML template element https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template – but it's not very widely adopted yet. |
@trieloff I agree that templating languages should be a user choice. I don't necessarily believe that having something very minimal default in the boilerplate is incompatible with that. My perspective is that it would maybe help in that sense with the pitfall of jumping to a heavy framework for the basic/common usecases and then maybe struggling to obtain a good LHS. Still users which have a different view of what they want to use, can do it without any barriers. Being in a separate file it won't even be loaded by the browser unless used and can be easily and cleanly discarded if not needed. The way I personally look at this code, is a helper function like You guys (the core team) have the best overview of all projects and usages, I just thought it might be helpful for others given the feedback from our project and the what I've found in others. |
Sure, I should've maybe been explicit: I only pointed it to show that, although possible with string literals, the developer experience when writing it could be a little bit more seamless from my point of view, when it comes to the composability aspect.
Noted! Was not aware! Thank you!
Happy to discuss and thank you very much for the feedback! |
While I see the point on not putting this into boilerplate, as @trieloff pointed out it should be a user's choice. There seems to be a need for this. I'm aware two other projects built helpers similar like this one. We might add this to the block party instead of boilerplate. |
@mhaack Thank you for the feedback!
you are referring to https://github.com/adobe/helix-block-collection , correct? @trieloff does that sounds like a good approach to you?
just checking: are they captured in the Other Projects section in this PR? if not could you please share them so I can add those as well? |
i agree that the best place would be "block party" mentioned by @mhaack to get started. the "block party" is a new concept we came up with for community contributed blocks and snippets. i don't think the code should be in any particular repos but it lives where it is... i don't know the latest on process for including things in the block party. maybe it makes sense to track those as issues in https://github.com/adobe/helix-website. |
https://github.com/hlxsites/block-party seems like the best place for this. |
should we move this issue to |
@davidnuescheler @trieloff Thank you! Created hlxsites/block-party#1 . Do you mind if I also keep this PR open as well for discoverability reasons? At the moment, not that many people know about the Block Party repository, while I think many check the boilerplate pull requests for possible improvemennts. |
To solve the discoverability issue, you could submit a PR that updates the README with links to block collection and block party. |
@trieloff just to clarify I understood correctly: You mean a PR to the README of this repository (the boilerplate)? Sure, will do! |
i am closing this PR, this will still keep it on record of course. keeping it open against this repo is misleading. we originally had DOM manipulation utilities in boilerplate and removed them as the produce an additional onboarding hurdle for new devs and usually devs don't agree what they want their DOM manipulation libraries to look like (which is why there are so many :) ) |
Context
This PR adds a small JS file to help with DOM elements generation.
We've been using this in our project and it speeds up a lot the development, code reviews and testing, so I thought it might be useful for new projects as well.
hlxsites/moleculardevices#161
Why?
The following Dom Helper functions allow for:
Main Usecases
Examples
It allows rewrite from:
To:
More examples from our project code:
Other projects
The reason why I thought it would be a good addition to the boiler plate is that I found similar/partial implementations, some repeating between projects, some independently written.
Credits
This is of course inspired by the JSX (React) syntax, articles and code pieces explaining its internals.