-
Notifications
You must be signed in to change notification settings - Fork 187
Request groups page #1196
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
Request groups page #1196
Conversation
import { buildApiAction } from './base'; | ||
|
||
export const FetchGroups = buildApiAction( | ||
'FETCH_GROUPS', |
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.
i'd suggest being explicit with the naming (FetchRequestGroups
, FETCH_REQUEST_GROUPS
), we also have the concept of groups for authentication (which may be featured more prominently in the future)
@@ -36,6 +37,7 @@ const AppRouter = (props) => { | |||
<Route path="requests/new" component={RequestForm} /> | |||
<Route path="requests/edit/:requestId" component={RequestForm} /> | |||
<Route path="requests(/:state)(/:subFilter)(/:searchFilter)" component={RequestsPage} /> | |||
<Route path="group/:groupId" component={Group} store={props.store} /> |
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.
i dont think the store
field is necessary
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.
Yep you're right, leftover from an earlier experiment.
@@ -353,6 +362,7 @@ function mapStateToProps(state, ownProps) { | |||
deploy: state.api.deploy.data, | |||
taskHistory: state.api.taskHistoryForDeploy.data, | |||
isTaskHistoryFetching: state.api.taskHistoryForDeploy.isFetching, | |||
group: state.api.deploy.data.deploy && _.first(_.filter(state.api.requestGroups.data, (g) => _.contains(g.requestIds, state.api.deploy.data.deploy.requestId))), |
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.
I'm adding a linting check to prohibit one-letter variable names (See #1192). Might want to change g
to groupToCheck
, or something more descriptive.
LGTM |
Creates a new Group page (
/group/:groupId
) which shows details for all requests contained in it.Also adds a breadcrumb item for requests, deploys, and tasks which are part of a group.
The metadata button displays a modal with copyable infoboxes. (button only is only shown if the group has metadata)
@tpetr @wolfd @Calvinp