-
Notifications
You must be signed in to change notification settings - Fork 14
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
Feature addition #1
Conversation
SridarDhandapani
commented
Dec 21, 2016
- Configurable folder prefix for upload.
- Automatically set content-type for uploaded file.
- Keep recently added file at top of the pile for easy reach.
- Few minor operational improvements.
@@ -134,6 +134,9 @@ const handleFiles = (files) => { | |||
* If config is present, instantiates S3Service basing on that information. | |||
*/ | |||
mb.on('ready', () => { | |||
const template = [{label: 'Actions', submenu: [{role: "paste"}, {role: "quit"}]}]; | |||
const menu = Menu.buildFromTemplate(template); |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
@@ -134,6 +134,9 @@ const handleFiles = (files) => { | |||
* If config is present, instantiates S3Service basing on that information. | |||
*/ | |||
mb.on('ready', () => { | |||
const template = [{label: 'Actions', submenu: [{role: "paste"}, {role: "quit"}]}]; |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
@@ -2,7 +2,7 @@ const menubar = require('menubar'); | |||
const fs = require('fs'); | |||
const path = require('path'); | |||
const notifier = require('node-notifier'); | |||
const { ipcMain, clipboard } = require('electron'); | |||
const { ipcMain, clipboard, Menu } = require('electron'); |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
'destructuring binding' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
@@ -52,11 +53,13 @@ class S3Service { | |||
*/ | |||
uploadFile(fileName, data) { | |||
const uploadEventEmitter = new EventEmitter(); | |||
const mimeType = fileType(data); |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
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.
Bad Indentation
@@ -1,5 +1,6 @@ | |||
const AWS = require('aws-sdk'); | |||
const EventEmitter = require('events'); | |||
const fileType = require('file-type'); |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
@@ -3,7 +3,7 @@ const fs = require('fs'); | |||
* Default file path where credentials will be stored. | |||
* @type {string} | |||
*/ | |||
const configFilePath = './.awscredentials.json'; | |||
const configFilePath = __dirname + '/.awscredentials.json'; |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
"menubar": "^4.1.2", | ||
"node-notifier": "^4.6.0", | ||
"react": "^15.2.0", | ||
"react-dom": "^15.2.0", | ||
"request": "^2.72.0" | ||
"request": "^2.72.0", | ||
"file-type": "^3.9.0" |
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.
Bad indentation
@@ -18,10 +18,10 @@ | |||
"Amazon Web Services", | |||
"React" | |||
], | |||
"author": "Rafal Wilinski", | |||
"author": "Sridar Dhandapani", |
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.s3.upload({ | ||
Body: data, | ||
ContentType: mimeType.mime, |
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.
Same here
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 do not alter author
section of package.json
and adapt proper coding style.
The indentations are consistent on my vim editor. Let me check the tab
settings.
As for the author section, I assumed you will be cherry picking that part
when you merge. You don't want to be blamed for something I have done...
On Thu, 22 Dec 2016 at 10:36 Rafal Wilinski ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Please do not alter author section of package.json and adapt proper
coding style.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARQ7TtfMWqGfmsTwN7gR2LBpJLsJSlqUks5rKlKRgaJpZM4LTYwh>
.
--
Sridar Dhandapani
sridar@gmail.com
|
I'm using 2 spaces. When it comes to cherry picking I'd rather like to merge whole PR. |
Sorry, mistakenly closed. |
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.
👍