-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
export folder as md or text #1147
Conversation
Hi @mslourens , thanks for your contribution! Could you fix CI error? |
two tests are still failing (one of them is mine), but I don't know why, because on my local machine they both pass (both on Ubuntu and Windows 10). |
Hi @mslourens , sorry for long absence! |
@kazup01 no problem, I fixed the conflicts |
browser/main/SideNav/StorageItem.js
Outdated
@@ -10,6 +10,7 @@ import dataApi from 'browser/main/lib/dataApi' | |||
import StorageItemChild from 'browser/components/StorageItem' | |||
import eventEmitter from 'browser/main/lib/eventEmitter' | |||
import _ from 'lodash' | |||
const path = require('path') |
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.
Please use import
.
browser/main/SideNav/StorageItem.js
Outdated
label: 'Unlink Storage', | ||
click: (e) => this.handleUnlinkStorageClick(e) | ||
})) | ||
let menu = Menu.buildFromTemplate([ |
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.
Please use const
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.
btw, using Menu.buildFromTemplate
is so nice :)
* exportDir: String | ||
* } | ||
* ``` | ||
*/ |
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.
thank you for your jsdoc 🙏
const resolveStorageData = require('./resolveStorageData') | ||
const resolveStorageNotes = require('./resolveStorageNotes') | ||
const path = require('path') | ||
const fs = require('fs') |
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.
Please use import
.forEach(snippet => { | ||
const notePath = path.join(exportDir, `${snippet.title}.${fileType}`) | ||
console.log(notePath) | ||
fs.writeFile(notePath, snippet.content, (err) => { |
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.
this will work as an async function
please handle it or use fs.writeFileSync
}) | ||
} | ||
|
||
module.exports = exportFolder |
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.
please use export default
I fixed most of the review comments, except the last one: if I use export default the index.js in the dataApi folder, where this file is required, will fail, because it won't load the correct file. Or do I something wrong? |
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.
LGTM
Merged. Thanks @mslourens ! I will fix CI error soon. |
implemented #1122