Skip to content
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

show delete confirmation dialog #1324

Merged
merged 5 commits into from
Jan 12, 2018
Merged

show delete confirmation dialog #1324

merged 5 commits into from
Jan 12, 2018

Conversation

mslourens
Copy link
Contributor

implements #1320

@BoostnoteBot
Copy link
Collaborator

Be sure to be changed browser/main/Detail/SnippetNoteDetail.js.

@kazup01 kazup01 requested a review from sosukesuzuki December 21, 2017 15:26
@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label Dec 21, 2017
@sferra
Copy link
Contributor

sferra commented Dec 21, 2017

@mslourens What do you think about shortening the message to simply "Move the note to the trash?" and changing the "Confirm" button text to "Move" for the sake of simplictity?

Also do you think that there is maybe some way to de-duplicate the code without any major refactoring?

@kazup01
Copy link
Member

kazup01 commented Dec 21, 2017

Hi @mslourens , thanks for your support . Could you fix CI error? https://travis-ci.org/BoostIO/Boostnote/jobs/319797454

@kazup01 kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 21, 2017
Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

please confirm my review.


if (isTrashed) {
const dialogueButtonIndex = dialog.showMessageBox(remote.getCurrentWindow(), {
Copy link
Member

Choose a reason for hiding this comment

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

dialog is never used.
I think import is unnecessary on line 31.

}, () => {
this.save()
})
}
}
ee.emit('list:next')
Copy link
Member

Choose a reason for hiding this comment

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

I want you to make don't call ee.emit('line:next') when I push cancel.
(please fix it too on SnippetNoteDetail.js.)

12 -23-2017 11-19-26

@mslourens
Copy link
Contributor Author

sorry for the delay, I haven't been working for some time, because of the holidays.

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

thanks, please confirm my reviews again.

@@ -28,7 +28,6 @@ import striptags from 'striptags'

const electron = require('electron')
const { remote } = electron
Copy link
Member

Choose a reason for hiding this comment

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

remote and electron are unnecessary.

}
}
ee.emit('list:next')

Copy link
Member

Choose a reason for hiding this comment

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

please remove this blank line.

@sosukesuzuki
Copy link
Member

Sorry for delay.
I approved! LGTM!

@kazup01 kazup01 merged commit 85937d8 into BoostIO:master Jan 12, 2018
@kazup01
Copy link
Member

kazup01 commented Jan 12, 2018

Merged. Thanks @mslourens !

@kazup01 kazup01 added next release (v0.8.20) and removed Next Release awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Jan 12, 2018
@Rokt33r Rokt33r mentioned this pull request Jan 13, 2018
@mslourens mslourens deleted the confirmation-dialog branch January 17, 2018 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants