Endpoint to allow users to delete pending on-demand tasks #1653
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import React, { Component, PropTypes } from 'react'; | ||
|
|
||
| import { Glyphicon } from 'react-bootstrap'; | ||
| import OverlayTrigger from 'react-bootstrap/lib/OverlayTrigger'; | ||
| import ToolTip from 'react-bootstrap/lib/Tooltip'; | ||
|
|
||
| import { getClickComponent } from '../modal/ModalWrapper'; | ||
|
|
||
| import DeletePendingTaskModal from './DeletePendingTaskModal'; | ||
|
|
||
| const deletePendingTaskTooltip = ( | ||
| <ToolTip id="delete"> | ||
| Delete this pending task | ||
| </ToolTip> | ||
| ); | ||
|
|
||
| export default class DeletePendingTaskButton extends Component { | ||
|
|
||
| static propTypes = { | ||
| taskId: PropTypes.string.isRequired, | ||
| requestType: PropTypes.string.isRequired, | ||
| then: PropTypes.func | ||
| }; | ||
|
|
||
| static defaultProps = { | ||
| children: ( | ||
| <OverlayTrigger placement="top" id="view-delete-pending-task-overlay" overlay={deletePendingTaskTooltip}> | ||
| <a> | ||
| <Glyphicon glyph="trash" /> | ||
| </a> | ||
| </OverlayTrigger> | ||
| ) | ||
| }; | ||
|
|
||
| render() { | ||
| if (this.props.requestType == "ON_DEMAND") { | ||
|
There was a problem hiding this comment. Rather than making this check here, I would only render the button in the first place if the type is There was a problem hiding this comment. Since the There was a problem hiding this comment. You could wrap it in a conditional, like |
||
| return ( | ||
| <span> | ||
| {getClickComponent(this)} | ||
| <DeletePendingTaskModal | ||
| ref="modal" | ||
| taskId={this.props.taskId} | ||
| requestType={this.props.requestType} | ||
| then={this.props.then} | ||
| /> | ||
| </span> | ||
| ); | ||
| } | ||
| return (null); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import React, { Component, PropTypes } from 'react'; | ||
| import { connect } from 'react-redux'; | ||
|
|
||
| import { DeletePendingOnDemandTask } from '../../../actions/api/tasks'; | ||
|
|
||
| import FormModal from '../../common/modal/FormModal'; | ||
|
|
||
| class DeletePendingTaskModal extends Component { | ||
| static propTypes = { | ||
| taskId: PropTypes.string.isRequired, | ||
| requestType: PropTypes.string.isRequired, | ||
| deletePendingTask: PropTypes.func.isRequired, | ||
| then: PropTypes.func | ||
| }; | ||
|
|
||
| show() { | ||
| this.refs.deletePendingTaskModal.show(); | ||
| } | ||
|
|
||
| render() { | ||
| return ( | ||
| <FormModal | ||
|
There was a problem hiding this comment. Do we not already have a confirm modal? Feels bad doing it as a There was a problem hiding this comment. Ah yes, we do have one! I'll swap it out. |
||
| name="Delete Pending Task" | ||
| ref="deletePendingTaskModal" | ||
| action="Delete Pending Task" | ||
| onConfirm={() => this.props.deletePendingTask()} | ||
| buttonStyle="primary" | ||
| formElements={[]}> | ||
| <p>Are you sure you want to delete this task?</p> | ||
| <pre>{this.props.taskId}</pre> | ||
| </FormModal> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const mapDispatchToProps = (dispatch, ownProps) => ({ | ||
| deletePendingTask: () => dispatch(DeletePendingOnDemandTask.trigger(ownProps.taskId)).then(response => (ownProps.then && ownProps.then(response))) | ||
| }); | ||
|
|
||
| export default connect( | ||
| null, | ||
| mapDispatchToProps, | ||
| null, | ||
| { withRef: true } | ||
| )(DeletePendingTaskModal); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,8 @@ master: | |
| net: host | ||
| environment: | ||
| MESOS_ZK: zk://localhost:2181/mesos | ||
| MESOS_HOSTNAME: 192.168.99.100 | ||
|
There was a problem hiding this comment. Have a fix for this in #1655 , I'll merge through to master so you can pull it in |
||
| MESOS_IP: 192.168.99.100 | ||
| MESOS_QUORUM: 1 | ||
| MESOS_CLUSTER: docker-compose | ||
| MESOS_WORK_DIR: /var/lib/mesos | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Defining the children here seems weird, especially since the component is only being used in one place. Do we need the ability to override the button's contents? Either way, I would move the logic into the
renderfunction rather thandefaultProps.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.
No override necessary that I can think of. Curious, why is it done this way in the
PauseButton?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 not sure, this UI has a lot of demons ha. I don't blame you for copying it though. In this case I think it's fine if you just hardcode it.
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.
Ahh, figured out why it's defined here - the
getClickComponenthooks up theonClicklogic based onprops.children. I'll leave for now unless you have better alternative?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.
That's fine if it'll be a bigger refactor. Better to have it be consistent for now.