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
Permalink request within group #1748
Conversation
@@ -1,5 +1,6 @@ | |||
import React, {PropTypes, Component} from 'react'; | |||
import { connect } from 'react-redux'; | |||
import { LinkContainer } from 'react-router-bootstrap'; |
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.
What's the reasoning behind using LinkContainer
over the standard react-router Link
or an onclick handler?
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.
Link
doesn't play nice with bootstrap components. I also wanted the URL to change when a user clicked on a request ID which is why I went with LinkContainer
. Would it have been better to have an onClick handler and wrap the component using withRouter
?
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.
If we were already using react-router-bootstrap
I'd say this is the best solution, however, I'm hesitant to add another dependency for this single use case since we already have so many. For that reason I'd say go with handling it via onClick
and withRouter
.
</NavItem> | ||
<LinkContainer | ||
eventKey={requestId} | ||
key={index} |
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.
requestId
is a logical key here
@@ -13,7 +14,7 @@ class GroupDetail extends Component { | |||
constructor(props) { | |||
super(props); | |||
this.state = { | |||
showRequestId: _.first(props.group.requestIds) | |||
showRequestId: props.params.requestId || _.first(props.group.requestIds), |
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.
component state isn't needed anymore, and you can get rid of onSelect={this.handleRequestSelect}
on the Nav component
|
Previously, request groups didn't support permalinks for requests within the group. It would simply select the first request and display that.
You'd have link that looked like this:
Which would contain some requests: RequestGroup1-foo1, RequestGroup1-foo2, RequestGroup1-foo3
Now you can link them directly:
The default behavior is maintained, no request ID means display the first request. Incorrect request ID will still load the selected group page, but the request data will not be displayed since there is none.