-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
Attribute hook render option #307
Conversation
|
Hey, isn't this possible with using import { options } from 'preact';
const oldVnodeHook = options.vnode
options.vnode = (vnode) => {
// is it a dom node?
if (vnode.type === 'string') {
const newProps = {};
// iterate over props and do stuff
vnode.props = newProps
}
if (oldVnodeHook) oldVnodeHook(vnode);
} |
Agree, to me it feels like that use case is already covered by |
It doesn't really work well.
|
@gpoitch Your codesandbox has a couple of issues and unnecessary cloning. The I'm not sure why the Here is a cleaned up version of the codesandbox: https://codesandbox.io/s/intelligent-heisenberg-7h6ysl?file=/src/index.js
I guess the more interesting question we should ask here is: Why do you want to uppercase the attribute names in the first place? I think if you can present a case where this is needed we're more than happy to add it. Can you share more about that? |
@marvinhagemeister the uppercasing was just to keep the example simple. Real use-cases for this off the top of my head:
|
That's a fair point. Are those readers common? And is the spec case sensitive too or is it more a reader not following the spec?
Do you have an example of an SVG that renders incorrectly?
Do you have an example where AMP validation fails?
To me that sounds like a bug we should fix. |
Yes a media company that runs on this with 10s of millions of daily views 😄. I can put together examples if you really want but there is an entire class of content rendering that may not have preact/the browser "correcting" it. I'd happily use the vnode option if it's viable but the already handled special cases and option re-modification doesn't look promising. We will continue to run the fork, just thought I'd share the idea since there are several issues about this always popping up (#26, #275, #18, #55). Thanks for taking the time to look at this. |
It's great to hear that you have successful clients. I was asking for a technical example. I don't work frequently with RSS readers so my question was more about if that's spec thing in that it only allows uppercase attribute names or if the spec is case insensitive and the readers implemented it incorrectly. Regarding #26, #275, #18, #55: I'd rather fix those issues in |
Nothing uses uppercase. Again, that was for a simple example's sake. All of the real transformations I've encountered are in the tests and #199 The technical examples are literally every single RSS, SVG, and AMP page you write in JSX and render with this library. If the RSS doesn't fully validate, clients/apps won't accept your content. If the AMP doesn't validate, google won't put you in search. If the SVG doesn't have proper attributes it won't render correctly. Regarding the other issues and correcting the attributes directly, I already provide that in #199 and got no traction. We could do a combination of the 2 - provide this hook and either export or make this the default. If you have another direction in mind, I will gladly implement it. |
Just merged #308 which addresses the known HTML + SVG scenarios. I'm not too familiar with AMP or RSS. Do they have additional cases to be aware of? |
For RSS: AMP should be handled with the HTML scenarios |
Adds an attribute hook rendering option. This allows the user to customize how attribute name casing is rendered.
There have been several issues and attempts over years to implement this. Namely: #199
Seems the consensus is to do nothing because other frameworks don't and there isn't an agreeable set of regexes to handle everything, so instead let the user do it.
From my experience, the proper casing on some of these is absolutely critical. We've been maintaining a fork for years, and couldn't keep up syncing upstream improvements after v6 so this is a more non-invasive approach.