Skip to content

Add export as txt/md #245

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

Merged
merged 4 commits into from
Jan 21, 2017
Merged

Add export as txt/md #245

merged 4 commits into from
Jan 21, 2017

Conversation

asmsuechan
Copy link
Contributor

@asmsuechan asmsuechan commented Jan 16, 2017

Hi, I implemented a function which exports as txt or md. I heard it was said that someone wants this function.

2017-01-16 13 03 13

@kazup01
Copy link
Member

kazup01 commented Jan 16, 2017

That’s awesome! We'll check it. Cheers👏

@kazup01 kazup01 requested a review from sota1235 January 16, 2017 04:14
@@ -6,6 +6,8 @@ import consts from 'browser/lib/consts'
import Raphael from 'raphael'
import flowchart from 'flowchart'
import SequenceDiagram from 'js-sequence-diagrams'
import ee from 'browser/main/lib/eventEmitter'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not omit the name of variable. For example, use eventEmitter.

@@ -25,6 +27,7 @@ function decodeHTMLEntities (text) {
const { remote } = require('electron')
const { app } = remote
const path = require('path')
const dialog = remote.require('electron').dialog
Copy link
Contributor

Choose a reason for hiding this comment

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

electron seems to be required twice.
If you import it on the first line, you don't need require twice.

import { remote, dialog } from 'electron'

Copy link
Contributor Author

@asmsuechan asmsuechan Jan 17, 2017

Choose a reason for hiding this comment

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

OMG, I don't have to require twice 😨. However the code cannot operate correctly because I want to use dialog in the renderer process. Thus I'll change it to const dialog = remote.dialog

The Dialog is opened from Electron's main thread. If you want to use the dialog object from a renderer process, remember to access it using the remote:

const {dialog} = require('electron').remote
console.log(dialog)

refs: https://github.com/electron/electron/blob/master/docs/api/dialog.md

dialog.showSaveDialog(remote.getCurrentWindow(), options,
(filename) => {
if (filename) {
fs.writeFile(filename, this.props.value, () => {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Please handle error by using callback function:bow:

submenu: [
{
label: 'Plain Text (.txt)',
click: function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can write like below. It is more modern way.

click () {
}

@sota1235
Copy link
Contributor

Thank you for your contribution! Please fix some points 🐱

@asmsuechan
Copy link
Contributor Author

@sota1235 Thanks so much for your review 🙏 🙏 🙏, I'll fix them 🙏 🙏 🙏

@asmsuechan
Copy link
Contributor Author

I fixed them 🙏

@sota1235
Copy link
Contributor

Thank you for fixing!
I want you to fix last one point.
If I try to export file when Snippet note displayed, it won't happen nothing.
I think that it would be more helpful to have a guide.
For example, show alert message Markdown note only support!

@asmsuechan
Copy link
Contributor Author

Thanks for you review so much 🙏 I'll handle it!

@asmsuechan
Copy link
Contributor Author

For example, show alert message Markdown note only support!

Hi, I handled it.

2017-01-20 18 58 03

@@ -305,6 +310,20 @@ class NoteList extends React.Component {
})
}

alertIfSnippet() {
let { location } = this.props
let targetIndex = _.findIndex(this.notes, (note) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use const instead of let!

alertIfSnippet() {
let { location } = this.props
let targetIndex = _.findIndex(this.notes, (note) => {
return note.storage + '-' + note.key === location.query.key
Copy link
Contributor

@sota1235 sota1235 Jan 21, 2017

Choose a reason for hiding this comment

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

You can use template literal!

return `${note.storage}-${note.key}` === location.query.key

@sota1235
Copy link
Contributor

Tha's all from me. Please fix, then I'll merge 🙇

@sota1235 sota1235 removed their assignment Jan 21, 2017
@asmsuechan
Copy link
Contributor Author

@sota1235 Thanks for your review! I fixed the code you pointed 🙏

@sota1235
Copy link
Contributor

Thank you for fixing. Looks good to me!

@sota1235 sota1235 merged commit 7201a98 into BoostIO:master Jan 21, 2017
@asmsuechan asmsuechan deleted the add-md-and-text-exporter branch January 21, 2017 06:56
@sota1235 sota1235 mentioned this pull request Jan 21, 2017
@kazup01 kazup01 mentioned this pull request Apr 24, 2017
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.

3 participants