navlink performance improvement, provide createNavLink, remove handleRoutes #70
Conversation
|
|
||
| describe('componentWillUnmount', function () { | ||
| it('should update active state', function () { | ||
| var div = document.createElement('div'); |
There was a problem hiding this comment.
Any reason this test was removed? I think the test was misnamed, but should still be valid right?
There was a problem hiding this comment.
when we remove handleRoute (also connectToStore) I think we no longer listen to RouteStore change and remove listener
|
updated, now provide a function for users to overwrite attributes and create a NavLink component |
kaesonho
changed the title
|
updated, still listen to the RouteStore change. |
|
CLA is valid! |
| return false; | ||
| }, | ||
| componentWillReceiveProps: function (nextProps) { | ||
| this.receivedNewProps = true; |
There was a problem hiding this comment.
We want to prevent the re-render for route store change, (only render when isActive change), while we still need to re-render for props change, so set a flag receivedNewProps and clear it after rendered.
|
lgtm |
kaesonho
force-pushed
the
refactornavlink
branch
from
72a10f4
to
2f0b715
Compare
|
thanks 🎆, squashed |
|
👍 |
| }, | ||
| propTypes: { | ||
| currentRoute: React.PropTypes.object, | ||
| currentNavigate: React.PropTypes.object, |
There was a problem hiding this comment.
Who will populate currentRoute and currentNavigate props, after handleRoute is removed?
|
do we want to put |
|
Keep it in |
kaesonho
changed the title
|
updated, change file name as |
|
|
||
| | Param Name | Param Type | Description | | ||
| |-----------|-----------|-------------| | ||
| | options | Object | the object taken to overwrite the default attribute when we create NavLink | |
There was a problem hiding this comment.
It would be good to document what these options are. Maybe with | options.foo | Object | <description> |.
There was a problem hiding this comment.
the description will be the same as React API, maybe give users a link http://facebook.github.io/react/docs/component-specs.html ?
There was a problem hiding this comment.
Oh, I see. That wasn't clear to me. Yeah it might be good to say it's the same as the parameter passed to React.createClass.
There was a problem hiding this comment.
sure updated, also update the parameter name, use overwriteSpec, which align with React, https://github.com/facebook/react/blob/master/src/isomorphic/classic/class/ReactClass.js#L804
|
👍 |
|
⛵ |
…eNavLinkComponent for users to customize it
kaesonho
force-pushed
the
refactornavlink
branch
from
b0309a0
to
b61bf31
Compare
|
rebased, let me merge it |
navlink performance improvement, provide createNavLink, remove handleRoutes
|
👍 |
Higher order component pattern is good but since users might use
NavLinkeverywhere, so havingconnectToStore -> handleRoute -> NavLink -> ashould be too much and the performance might be able to be improved.makePathandisActivefrom routeStore, use it directly, so that we can reduce two higher component and we can remove the eventListeners to the RouteStore. previously the RouteStore might propagate hundreds emitChange to every NavLink on the page.move out NavLink functions to benavLinkUtils, so that if users have the use case they can compose there own NavLink.provide
createNavLinkso that users can overwrite the attribute with whatever they want, maybe add a mixin or a custom click event handler@mridgway @redonkulus @lingyan @Vijar