-
Notifications
You must be signed in to change notification settings - Fork 529
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
fix(templates): ensure consistency in using and recommending function/htm templates #5583
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 456f7b8:
|
This allows html in the pagination template without it being unsafe.
This adds the name of the template which is in string form (and thus deprecated)
These are superseded by the components in html tagged templates
b3f4f3b
to
f3e13dc
Compare
f3e13dc
to
bf48535
Compare
<div class="hits-image" style="background-image: url(${hit.image})"></div> | ||
<article> | ||
<header> | ||
<strong>${components.Highlight({ hit, attribute: 'name' })}</strong> |
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.
Shouldn't we rather do this :
const hitsItemTemplate = (hit, { components: { Highlight } }) => html`
<strong><${Highlight} hit=${hit} attribute="name" /></strong>
`;
If we call the component as a function Preact won't be aware it's actually a component and won't show up in devtools.
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.
Yes, it could be interesting, but in our documentation we only document the "function call" as it's much simpler to read and doesn't require people to know about preact.
Why doesn't it show up in the devtools though? it's still a preact component right?
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.
Making a function call would be okay if these were just render functions but as they start with a capitalized letter and take a single prop argument, it's implied they are components which should be instantiated with Preact's h
as they could use hooks.
They won't show up in Devtools in the VDOM as Highlight
or Snippet
components because they won't ever get instantiated by Preact's h
function.
Now I know it's not a big deal yet as these components are in fact just render functions, but in the future if we provide any component using hooks this could be a problem :/
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.
We could also do something like this developit/htm#167 (comment) (allowing the components to be a string), but I think both of those points are a separate discussion from this PR (as the PR just makes consistent what we already document), what do you think @aymeric-giraudet?
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.
Ah yes this could be even more user-friendly !
Indeed it's a different discussion, but it's good we're already aware of the eventual shortcomings we may face in the future :D
I'll review the rest !
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.
#5584 it is possible, but I'm not convinced it's the way to go yet. We can do it easily if we want it.
c89b3f1
to
6c789b6
Compare
6c789b6
to
456f7b8
Compare
Summary
This fixes several places where we were either still having the "use html templates instead" warning by default, or wrongly documented.
Result
Each commit can be reviewed separately, they don't touch any overlapping files.