-
Notifications
You must be signed in to change notification settings - Fork 60
Enhancements in routing - Closes #499 #509
Changes from 7 commits
4276637
04c450e
8114e45
db33e6d
1c3ce57
d769567
edfef20
a3f6ad8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import React from 'react'; | ||
import { Link } from 'react-router-dom'; | ||
import Account from '../account'; | ||
import PrivateRoute from '../privateRoute'; | ||
|
||
const DefaultLayout = ({ component: Component, ...rest }) => ( | ||
<PrivateRoute {...rest} component={ matchProps => ( | ||
<main> | ||
<Account /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Links component was moved on @slaweet request. I think that the performance change here is negligible especially if you will move the logic from account component and keep it dumb and allow only render the passed props. I do not know how the app will be looked after rebranding, and I think that the layout can help combine some components for differents views. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed you render Account component and Link components every time changing the route, I think rending them once will help improving performance. to do so, I’d suggest find a way to keep those components mounted once or adapting unit tests. nested routes are used in many apps. there should be some other applications facing this situation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yasharAyari fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good job. thank you. |
||
<Link to='/transactions'>Transactions</Link> | ||
<Link to='/voting'>Voting</Link> | ||
<Link to='/forging'>Forging</Link> | ||
<Component {...matchProps} /> | ||
</main> | ||
)} /> | ||
); | ||
|
||
export default DefaultLayout; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import React from 'react'; | ||
import { Route, Redirect, withRouter } from 'react-router-dom'; | ||
import { connect } from 'react-redux'; | ||
|
||
export const PrivateRouteComponent = ({ component: Component, isAuthenticated, ...rest }) => ( | ||
<Route {...rest} render={ matchProps => ( | ||
isAuthenticated ? ( | ||
<Component {...matchProps}/> | ||
) : | ||
<Redirect to='/' /> | ||
)}/> | ||
); | ||
|
||
const mapStateToProps = state => ({ | ||
isAuthenticated: !!state.account.publicKey, | ||
}); | ||
|
||
export default withRouter(connect(mapStateToProps)(PrivateRouteComponent)); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import React from 'react'; | ||
import { expect } from 'chai'; | ||
import { mount } from 'enzyme'; | ||
import { MemoryRouter, Route } from 'react-router'; | ||
import { PrivateRouteComponent } from './index'; | ||
|
||
const Public = () => <h1>Public</h1>; | ||
const Private = () => <h1>Private</h1>; | ||
|
||
describe('<PrivateRoute />', () => { | ||
const isAuth = isAuthenticated => ( | ||
mount( | ||
<MemoryRouter initialEntries={['/private']}> | ||
<div> | ||
<Route path='/' component={Public} /> | ||
<PrivateRouteComponent | ||
path='/private' | ||
component={Private} | ||
isAuthenticated={isAuthenticated} /> | ||
</div> | ||
</MemoryRouter>, | ||
) | ||
); | ||
it('should render Component if user is authenticated', () => { | ||
const wrapper = isAuth(true); | ||
expect(wrapper.find(Private)).to.have.length(1); | ||
}); | ||
|
||
it('should redirect to root path if user is not authenticated', () => { | ||
const wrapper = isAuth(false); | ||
expect(wrapper.find(Public)).to.have.length(1); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,7 +70,7 @@ module.exports = (env) => { | |
exclude: /node_modules/, | ||
loader: 'babel-loader', | ||
options: { | ||
presets: ['es2015', 'react'], | ||
presets: ['es2015', 'react', 'stage-3'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I see it is because of |
||
plugins: ['syntax-trailing-function-commas'], | ||
env: { | ||
test: { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the
skip
or add a comment on why it needs to be skipped