Skip to content

#381 - listen to events on demand#462

Closed
SanderSpies wants to merge 47 commits into
facebook:masterfrom
SanderSpies:on-demand-events
Closed

#381 - listen to events on demand#462
SanderSpies wants to merge 47 commits into
facebook:masterfrom
SanderSpies:on-demand-events

Conversation

@SanderSpies
Copy link
Copy Markdown
Contributor

@SanderSpies SanderSpies commented Nov 1, 2013 •
edited by sophiebits
Loading

Fixes #381.

Changes of @spicyj are now in.

Apologies for the overload of commits, had some git issues :-).

@zpao
Copy link
Copy Markdown
Contributor

zpao commented Nov 1, 2013

Let's ignore the window resize and whatever other events that React attaches and uses internally for now. If possible, it would be great to end up removing those entirely but that should probably be a separate effort.

petehunt
petehunt reviewed Nov 3, 2013
View reviewed changes
Comment thread src/core/ReactEventEmitter.js Outdated
Copy link
Copy Markdown
Contributor

@petehunt petehunt Nov 3, 2013

Choose a reason for hiding this comment

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

React codebase convention is to have a single var statement for each declaration.

@SanderSpies
Copy link
Copy Markdown
Contributor Author

SanderSpies commented Nov 7, 2013

9:41:05 PM sspi I removed touchNotMouse, and I want to re-add it
9:41:28 PM sspi but I want to add it at the eventPlugin level
9:44:33 PM sspi that way you can set dependencies the right way, instead of having dependencies on both mouse and touch
9:48:06 PM balpert sspi: that makes sense to me
9:48:17 PM balpert SimpleTouchEventPlugin or something
9:51:12 PM sspi balpert: I was thinking more in the direction of having a useTouchEvents available within the eventPlugin
9:51:21 PM sspi s
9:53:26 PM balpert yeah, you could do either
9:56:04 PM sspi ok
9:56:09 PM balpert if it works, I think extracting to a separate plugin makes a lot of sense
10:04:26 PM sspi balpert: the plugin would basically do what initializeTouchEvents does now?
10:05:35 PM sspi which is setting useTouchEvents
10:07:09 PM balpert no, I meant that SimpleEventPlugin would never listen to touch events
10:07:18 PM balpert and if you want touch events you need to add a different plugin
10:07:26 PM sspi so no TapEventPlugin?
10:09:10 PM balpert TapEventPlugin would depend on TouchEventPlugin?
10:09:15 PM balpert if you can do that?
10:09:19 PM balpert I didn't look at your code yet
10:09:51 PM sspi nah, don't think it can with the current solution :)

11:02:11 PM sspi balpert: I'm having second thoughts on the eventplugin dependencies
11:02:29 PM sspi to other eventplugins
11:04:06 PM sspi even with these dependencies there needs to be some mechanism to decide which events need to be loaded and which not
11:07:01 PM balpert right, it was not 100% clear in my mind when I was thinking about it a month or so ago
11:07:06 PM balpert which is why I never finished my diff
11:07:32 PM balpert :)

SanderSpies
SanderSpies reviewed Nov 8, 2013
View reviewed changes
Comment thread src/eventPlugins/ChangeEventPlugin.js Outdated
Copy link
Copy Markdown
Contributor Author

@SanderSpies SanderSpies Nov 8, 2013

Choose a reason for hiding this comment

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

Move ] to next line

@SanderSpies
Copy link
Copy Markdown
Contributor Author

SanderSpies commented Nov 19, 2013

Had a discussion with @spicyj about testing events to ensure that they are happening. This would be really nice to have, however I would prefer to add these types of tests within a separate PR.

SanderSpies and others added 4 commits November 20, 2013 07:21
@SanderSpies
Merge branch 'master' of https://github.com/facebook/react into on-de…
38b983e 
…mand-events
@sophiebits @SanderSpies
Make ReactEventTopLevelCallback-test pass
104a249 
This module-level mock() seems to have been interfering with other tests (26 specs failing). We don't have any other tests that do a module-level mock() so I'm going to assume it isn't supported in the open-source test runner right now. I changed it so only ReactEventEmitter.handleTopLevel is mocked; doing so made ReactEventEmitter complain that TopLevelCallbackCreator wasn't defined so I switched the ReactMount references to use React directly so that ReactDefaultInjection would kick in properly.

With this, all the tests pass.
@SanderSpies
- Implemented tests
8ef5049 
- Implemented EventPlugin dependencies
- Moved supportTouch to EventPlugin level
@SanderSpies
Merge branch 'master' of https://github.com/facebook/react into on-de…
3efd107 
…mand-events
sophiebits
sophiebits reviewed Nov 28, 2013
View reviewed changes
Comment thread src/core/React.js Outdated
Copy link
Copy Markdown
Collaborator

@sophiebits sophiebits Nov 28, 2013

Choose a reason for hiding this comment

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

style nit: requires should always be alphabetized.

yungsters
yungsters reviewed Dec 19, 2013
View reviewed changes
Comment thread src/core/ReactDOMComponent.js Outdated
Copy link
Copy Markdown
Contributor

@yungsters yungsters Dec 19, 2013

Choose a reason for hiding this comment

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

Could the logic here to find the container and execute listenTo be put into ReactEventEmitter.putListener? Then it doesn't even have to be exposed on ReactEventEmitter and could be re-used below.

@yungsters
Copy link
Copy Markdown
Contributor

yungsters commented Dec 19, 2013

Added a bunch of comments — some are from me, some are from @petehunt. Thanks for tackling this!

@SanderSpies
Copy link
Copy Markdown
Contributor Author

SanderSpies commented Dec 20, 2013

@yungsters @petehunt - thanks for the review.

@ghost ghost assigned yungsters Dec 31, 2013
@plievone plievone mentioned this pull request Jan 2, 2014
Closed
zpao pushed a commit that referenced this pull request Jan 4, 2014
@SanderSpies @zpao
Listen to events on demand
80d7d2d 
Fixes #381

This is a squashed version of #462
@zpao
Copy link
Copy Markdown
Contributor

zpao commented Jan 4, 2014

And 🚢ed! 80d7d2d

@zpao zpao closed this Jan 4, 2014
@jordwalke
Copy link
Copy Markdown
Contributor

jordwalke commented Jan 4, 2014

Awesome!

plievone
plievone reviewed Jan 9, 2014
View reviewed changes
Comment thread src/core/ReactEventEmitter.js
Copy link
Copy Markdown
Contributor

@plievone plievone Jan 9, 2014

Choose a reason for hiding this comment

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

There are a few lines over 80 chars, this is one

toptaldev92 pushed a commit to toptaldev92/react_project that referenced this pull request Jul 28, 2021
@SanderSpies @toptaldev92
Listen to events on demand
a6a61f2 
Fixes #381

This is a squashed version of facebook/react#462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@yungsters yungsters

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Listen for events on demand instead of on first mount

8 participants

@SanderSpies @zpao @sophiebits @yungsters @jordwalke @plievone @vjeux @petehunt