Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Download Item Removal Confirmation #7072

Merged
merged 1 commit into from
Feb 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion app/renderer/components/downloadItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class DownloadItem extends ImmutableComponent {
this.onResumeDownload = this.onDownloadActionPerformed.bind(this, RESUME)
this.onCancelDownload = this.onDownloadActionPerformed.bind(this, CANCEL)
this.onClearDownload = this.onClearDownload.bind(this)
this.onShowDeleteConfirmation = this.onShowDeleteConfirmation.bind(this)
this.onHideDeleteConfirmation = this.onHideDeleteConfirmation.bind(this)
this.onDeleteDownload = this.onDeleteDownload.bind(this)
this.onRedownload = this.onRedownload.bind(this)
this.onCopyLinkToClipboard = this.onCopyLinkToClipboard.bind(this)
Expand All @@ -34,7 +36,14 @@ class DownloadItem extends ImmutableComponent {
onClearDownload () {
appActions.downloadCleared(this.props.downloadId)
}
onShowDeleteConfirmation () {
appActions.showDownloadDeleteConfirmation()
}
onHideDeleteConfirmation () {
appActions.hideDownloadDeleteConfirmation()
}
onDeleteDownload () {
appActions.hideDownloadDeleteConfirmation()
appActions.downloadDeleted(this.props.downloadId)
}
onDownloadActionPerformed (downloadAction) {
Expand Down Expand Up @@ -74,10 +83,17 @@ class DownloadItem extends ImmutableComponent {
return <span
onContextMenu={contextMenus.onDownloadsToolbarContextMenu.bind(null, this.props.downloadId, this.props.download)}
onDoubleClick={this.onOpenDownload}
onMouseLeave={this.onHideDeleteConfirmation}
className={cx({
downloadItem: true,
deleteConfirmationVisible: this.props.deleteConfirmationVisible,
[this.props.download.get('state')]: true
})}>
{
this.props.deleteConfirmationVisible
? <div className='deleteConfirmation'>Delete?<Button l10nId='ok' className='primaryButton confirmDeleteButton' onClick={this.onDeleteDownload} /></div>
: null
}
<div className='downloadActions'>
{
downloadUtil.shouldAllowPause(this.props.download)
Expand Down Expand Up @@ -111,7 +127,7 @@ class DownloadItem extends ImmutableComponent {
}
{
downloadUtil.shouldAllowDelete(this.props.download)
? <Button className='deleteButton' l10nId='downloadDelete' iconClass='fa-trash-o' onClick={this.onDeleteDownload} />
? <Button className='deleteButton' l10nId='downloadDelete' iconClass='fa-trash-o' onClick={this.onShowDeleteConfirmation} />
: null
}
{
Expand Down
1 change: 1 addition & 0 deletions app/renderer/components/downloadsBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class DownloadsBar extends ImmutableComponent {
.map((download, downloadId) =>
<DownloadItem download={download}
windowWidth={this.props.windowWidth}
deleteConfirmationVisible={this.props.deleteConfirmationVisible}
downloadId={downloadId}
downloadsSize={this.props.downloads.size} />)
}
Expand Down
12 changes: 12 additions & 0 deletions docs/appActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,18 @@ Dispatches a message when a download is being redownloaded



### showDownloadDeleteConfirmation()

Shows delete confirmation bar in download item panel



### hideDownloadDeleteConfirmation()

Hides delete confirmation bar in download item panel



### clipboardTextCopied(text)

Dispatches a message when text is updated to the clipboard
Expand Down
18 changes: 18 additions & 0 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,24 @@ const appActions = {
})
},

/**
* Shows delete confirmation bar in download item panel
*/
showDownloadDeleteConfirmation: function () {
AppDispatcher.dispatch({
actionType: appConstants.APP_SHOW_DOWNLOAD_DELETE_CONFIRMATION
})
},

/**
* Hides delete confirmation bar in download item panel
*/
hideDownloadDeleteConfirmation: function () {
AppDispatcher.dispatch({
actionType: appConstants.APP_HIDE_DOWNLOAD_DELETE_CONFIRMATION
})
},

/**
* Dispatches a message when text is updated to the clipboard
* @param {string} text - clipboard text which is copied
Expand Down
1 change: 1 addition & 0 deletions js/components/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -1298,6 +1298,7 @@ class Main extends ImmutableComponent {
this.props.windowState.getIn(['ui', 'downloadsToolbar', 'isVisible']) && this.props.appState.get('downloads') && this.props.appState.get('downloads').size > 0
? <DownloadsBar
windowWidth={window.innerWidth}
deleteConfirmationVisible={this.props.appState.get('deleteConfirmationVisible')}
downloads={this.props.appState.get('downloads')} />
: null
}
Expand Down
2 changes: 2 additions & 0 deletions js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ const appConstants = {
APP_DOWNLOAD_DELETED: _,
APP_DOWNLOAD_CLEARED: _,
APP_DOWNLOAD_REDOWNLOADED: _,
APP_SHOW_DOWNLOAD_DELETE_CONFIRMATION: _,
APP_HIDE_DOWNLOAD_DELETE_CONFIRMATION: _,
APP_ALLOW_FLASH_ONCE: _,
APP_ALLOW_FLASH_ALWAYS: _,
APP_FLASH_PERMISSION_REQUESTED: _,
Expand Down
6 changes: 6 additions & 0 deletions js/stores/appStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,12 @@ const handleAppAction = (action) => {
appState = appState.setIn(['sync', 'seedQr'], action.seedQr)
}
break
case appConstants.APP_SHOW_DOWNLOAD_DELETE_CONFIRMATION:
appState = appState.set('deleteConfirmationVisible', true)
break
case appConstants.APP_HIDE_DOWNLOAD_DELETE_CONFIRMATION:
appState = appState.set('deleteConfirmationVisible', false)
break
default:
}

Expand Down
29 changes: 26 additions & 3 deletions less/downloadBar.less
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@

&:hover {
height: 62px;
top: -24px;
top: -23px;

.downloadInfo .downloadArrow {
display: none;
Expand All @@ -60,6 +60,10 @@
.downloadActions {
display: none;
}

.deleteConfirmation {
display: none;
}
}

&.paused {
Expand All @@ -75,6 +79,11 @@
}
}

&.deleteConfirmationVisible:hover {
height: 97px;
top: -58px;
}

.downloadProgress {
background-color: @highlightBlue;
left: 0;
Expand Down Expand Up @@ -125,6 +134,22 @@
}
}
}

.deleteConfirmation {
line-height: 2;
border-bottom: 1px solid #CCC;
padding: 5px 0;
margin-bottom: auto 0 10px 0;
font-size: 12px;

.confirmDeleteButton {
font-weight: normal;
padding: 1px;
min-width: 50px;
float: right;
margin-right: -5px;
}
}
}
}

Expand All @@ -150,5 +175,3 @@
}
}
}


50 changes: 43 additions & 7 deletions test/unit/app/renderer/downloadItemTest.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */
/* global describe, before, after, it */
/* global describe, before, beforeEach, after, it */

const mockery = require('mockery')
const {mount} = require('enzyme')
Expand All @@ -25,6 +25,7 @@ const newDownload = (state) => Immutable.fromJS({
url: downloadUrl,
totalBytes: 104729,
receivedBytes: 96931,
deleteConfirmationVisible: false,
state
})

Expand All @@ -39,6 +40,7 @@ describe('downloadItem component', function () {
DownloadItem = require('../../../../app/renderer/components/downloadItem')
appActions = require('../../../../js/actions/appActions')
})

after(function () {
mockery.disable()
})
Expand All @@ -48,13 +50,19 @@ describe('downloadItem component', function () {
before(function () {
this.downloadId = uuid.v4()
this.download = newDownload(state)
this.result = mount(<DownloadItem downloadId={this.downloadId} download={newDownload(state)} />)
this.result = mount(
<DownloadItem
downloadId={this.downloadId}
download={this.download}
deleteConfirmationVisible={this.download.get('deleteConfirmationVisible')} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this was wrong before. Before, it was this.download.downloadDeleteConfirmationVisible. Accessing child objects from an Immutable object will return undefined.

I updated this to use the Immutable.js get method to fetch the variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I never worked with ImmutableJS before, so most of my code in there was just a blind guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalbe no worries- it's something that bites me all the time (because it fails silently). Frankly, I wish an error would be thrown when it's accessed that way. I can't count the # times I've debugged an issue relating to that 😄

)
})

const shouldProgressBarExist = [downloadStates.IN_PROGRESS, downloadStates.PAUSED].includes(state)
it('filename exists and matches download filename', function () {
assert.equal(this.result.find('.downloadFilename').text(), this.download.get('filename'))
})

const shouldProgressBarExist = [downloadStates.IN_PROGRESS, downloadStates.PAUSED].includes(state)
it(shouldProgressBarExist ? 'progress bar should exist' : 'progress bar should not exist', function () {
assert.equal(this.result.find('.downloadProgress').length, shouldProgressBarExist ? 1 : 0)
})
Expand Down Expand Up @@ -114,11 +122,39 @@ describe('downloadItem component', function () {
})

testButton('.deleteButton', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
const spy = sinon.spy(appActions, 'downloadDeleted')
button.simulate('click')
assert(spy.withArgs(this.downloadId).calledOnce)
appActions.downloadDeleted.restore()
const spy = sinon.spy(appActions, 'showDownloadDeleteConfirmation')
try {
// Confirmation should NOT be visible by default
assert.equal(this.result.find('.confirmDeleteButton').length, 0)

// Clicking delete should show confirmation
button.simulate('click')
assert(spy.called)
} finally {
appActions.showDownloadDeleteConfirmation.restore()
}
})

if ([downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED].includes(state)) {
describe('when delete button has been clicked', function () {
beforeEach(function () {
this.result.setProps({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THIS is the part I missed! Thanks!

deleteConfirmationVisible: true
})
})

testButton('.confirmDeleteButton', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
const spy = sinon.spy(appActions, 'downloadDeleted')
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there's a try block in here?

Copy link
Member

@bsclifton bsclifton Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case something fails. For example, with your original commit, it failed because 0 items were found. This raised an exception which then did not properly restore the sinon spy. When the next test case ran, it would try to put a spy on the same method but fail (because spy has already been done). The try is required only because I wanted to use a finally to restore the method which had been spied

// Accepting confirmation should delete the item
button.simulate('click')
assert(spy.withArgs(this.downloadId).calledOnce)
} finally {
appActions.downloadDeleted.restore()
}
})
})
}
})
})
})