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

Allow custom attribute named on to be passed on to elements #11153

Merged

Conversation

nuc
Copy link
Contributor

@nuc nuc commented Oct 9, 2017

Hi there,

It seems that currently is not possible to pass a custom property named on on custom components.

We would like to render AMP pages on the server, in which you define the event handlers like so: <amp-image on="tap:do-something" />.

In order to do so, I made the check a bit more specific, to allow the property to be passed if it's just named on.

Even though the tests are passing, please let me know if that would be the right approach.

Thanks,
Georgios

@facebook-github-bot
Copy link

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

@nuc
Copy link
Contributor Author

nuc commented Oct 9, 2017

@facebook-github-bot Thank you dear bot. Email sent!

@nuc
Copy link
Contributor Author

nuc commented Oct 9, 2017

Hm, but then since we are on server side rendering, shouldn't this check prevent the need for the fix above - taken that the regex is loosened a bit?

Not sure how could I avoid the loading of any event plugin though, so the condition would pass.

That will allow us to attach on on other elements as well, which in fact is the most frequent case needed in AMP (<button on="tap:do-stuff">).

Or maybe that would be a total misuse of that condition.. so yeah, any input would be really much appreciated.

Cheers

spyOn(console, 'error');
var container = document.createElement('div');
ReactDOM.render(<amp-image on="tap" />, container);
expectDev(console.error.calls.count()).toBe(0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this (or spyOn call). By default we assume none of the tests warn unless they opt in.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead you'll want to assert that the on attribute gets set (which it probably doesn't right now).

@gaearon
Copy link
Collaborator

gaearon commented Oct 9, 2017

If you add an assertion about attribute presence, you'll probably see that this just silenced the warning but doesn't actually affect the output.

I think the real place to change this behavior is here.

@nuc
Copy link
Contributor Author

nuc commented Oct 9, 2017

Thanks, I'll prepare something.

@gaearon
Copy link
Collaborator

gaearon commented Oct 9, 2017

Also, ReactDOMServerIntegration-test might be a better place for this test since it's more extensive and covers SSR too.

@nuc
Copy link
Contributor Author

nuc commented Oct 9, 2017

@gaearon Something like this?

@@ -205,7 +205,8 @@ var DOMProperty = {
}
if (
(name[0] === 'o' || name[0] === 'O') &&
(name[1] === 'n' || name[1] === 'N')
(name[1] === 'n' || name[1] === 'N') &&
name.toLowerCase() !== 'on'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just check name.length instead?

@gaearon
Copy link
Collaborator

gaearon commented Oct 9, 2017

If this is only meant for custom elements, why are we testing with a DIV?

@nuc
Copy link
Contributor Author

nuc commented Oct 9, 2017

As I realised afterwards, in AMP it is also needed for standard elements as well, for example: <button on="tap:do-something" />.

That should not have any implications though, right?

I will update the PR title, so it reflects it.

@nuc nuc changed the title Allow single on property for custom elements Allow custom attribute named on to be passed on to elements Oct 9, 2017
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sebmarkbage sebmarkbage merged commit 65b16b1 into facebook:master Oct 10, 2017
@sebmarkbage
Copy link
Collaborator

Seems legit.

@gaearon gaearon mentioned this pull request Oct 10, 2017
19 tasks
@nuc nuc deleted the allow-on-property-on-custom-elements branch October 10, 2017 06:49
@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2017

React 16.1.0-beta has been released. Please update react, react-dom, and react-test-renderer (if you use it) to this version and let us know if it solved the issue! We’d appreciate if you could test before Monday when we plan to get 16.1.0 out.

@nuc
Copy link
Contributor Author

nuc commented Nov 3, 2017

@gaearon It works great. Thanks guys!

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