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.

Sign up for GitHub

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

Jump to bottom

Add onToggle event to details tag #8831

Merged
merged 4 commits into from Feb 3, 2017
Merged

Add onToggle event to details tag #8831

merged 4 commits into from Feb 3, 2017

Conversation

diegomura
Copy link
Contributor

Fixes #8761

diegomura
diegomura commented Jan 19, 2017
View reviewed changes
docs/docs/reference-events.md Outdated
@@ -338,3 +338,12 @@ string pseudoElement
float elapsedTime
```

* * *

### Misc Events
Copy link
Contributor Author

@diegomura diegomura Jan 19, 2017 •
edited

Choose a reason for hiding this comment

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

Not sure where to add this event on the docs. Does not feel right in any of the current categories, that's why I added a new one, but still not sure if is the way to go

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @gaearon for docs

Copy link
Contributor

Choose a reason for hiding this comment

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

Misc events is probably fine for now.

sebmarkbage
sebmarkbage reviewed Jan 20, 2017
View reviewed changes
src/renderers/dom/stack/client/ReactDOMComponent.js
@@ -331,6 +331,15 @@ function trapBubbledEventsLocal() {
),
];
break;
case 'details':
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will need to be replicated in ReactDOMFiberComponent which is a fork.

@sebmarkbage
Copy link
Collaborator

Looks fine to me but needs to be replicated in the Fiber version.

@diegomura
Copy link
Contributor Author

You're right @sebmarkbage . Thanks for pointing that out!. I'll have some time today to replicate that on ReactDOMFiberComponent

@diegomura
Copy link
Contributor Author

Thanks @aweary. Is there anything else I should do?

cc @gaearon @sebmarkbage

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

Looks good to me. Can I ask you to test that Fiber version actually works? You can run npm run build, then take build/react-dom-fiber.js and create a tiny example with it to make sure.

@diegomura
Copy link
Contributor Author

Sure, I will do it asap. Thanks!

@diegomura
Copy link
Contributor Author

I missed adding the details tag on setInitialProperties for ReactFiberComponent 😅 .
It works as expected for Stack and Fiber reconciler now.

feb-03-2017 01-16-53

Thanks!

@gaearon
Copy link
Collaborator

gaearon commented Feb 3, 2017

Thanks for checking!

@gaearon gaearon added this to the 15-lopri milestone Feb 3, 2017
@gaearon gaearon merged commit ad133f5 into facebook:master Feb 3, 2017
@diegomura diegomura deleted the toggle-event branch February 3, 2017 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebmarkbage sebmarkbage sebmarkbage left review comments

@aweary aweary aweary approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
15-lopri
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@diegomura @sebmarkbage @gaearon @aweary @facebook-github-bot