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

Accept React attributes on custom components #11217

Closed
wants to merge 4 commits into from

Conversation

nuc
Copy link
Contributor

@nuc nuc commented Oct 13, 2017

Hi there,

It seems that currently if you pass a React attribute to a custom component, it doesn't replace it with the html counterpart but just renders it as text.

For example:

<amp-img className="my-image" /> will render <amp-img classname="my-image" />.

Apparently, that's by design, since we found that there are tests explicitly checking this behaviour.

Do you really think that this should work the way it is? It feels a bit strange that for custom components you would need to use class instead of className.

(thanks @jacksbox for the pairing session 😃)

Cheers

@nuc
Copy link
Contributor Author

nuc commented Oct 13, 2017

It is quite interesting though that the issue is only affecting custom components with a dash on their name.

So, without the suggested fix:

<amp-img className="my-image" /> becomes <amp-img classname="my-image" />

but:

<amp className="my-image" /> becomes <amp class="my-image" />

@sebmarkbage
Copy link
Collaborator

I think className, style and tabIndex are the only ones that might make sense to normalize. What else?

@nuc
Copy link
Contributor Author

nuc commented Oct 14, 2017

@sebmarkbage Sorry, but I don't understand your question.

I think the issue affects all attributes defined in the HTMLDOMPropertyConfig: acceptCharset, className, htmlFor, httpEquiv.

How is style and tabIndex related?

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

The latest consensus on this is here: #11347 (comment).
We'd probably take a PR implementing that approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants