-
Notifications
You must be signed in to change notification settings - Fork 20
Linux windows file dir picking #112
Linux windows file dir picking #112
Conversation
Also, I just tested both |
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.
Some stylistic comments, I haven't pulled this down yet to test, but the structure of the code looks great.
Ready for merge after comments are addressed and I've finished testing.
app/js/actions/currentdirectory.js
Outdated
if (fps) { | ||
dispatch(setArtifactDir(fps[0])); | ||
let curProps = ['openFile', 'openDirectory']; | ||
let curTitle = 'Choose Artifact Directory or File'; |
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.
I know it was already like this, but let's change it to: Choose Directory or File to Import
(I've also added comments to make the other titles consistent with this new title-scheme)
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.
Sounds good, will do!
app/js/actions/currentdirectory.js
Outdated
message: 'What would you like to select?', | ||
calcelId: 2 | ||
}); | ||
if (callback === 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.
Normally I would want to use a callback, but since remote.dialog.showMessageBox
blocks the process there's not much point.
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.
Yeah, I tried using a callback at first but came to the same conclusion after reading the messageBox documentation.
app/js/actions/currentdirectory.js
Outdated
}; | ||
if (process.platform !== 'darwin') { | ||
// linux or windows | ||
const callback = remote.dialog.showMessageBox({ |
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.
callback
-> response
since this isn't a function.
app/js/actions/currentdirectory.js
Outdated
}); | ||
if (callback === 0) { | ||
curProps = ['openFile']; | ||
curTitle = 'Choose Artifact File'; |
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.
Choose File to Import
app/js/actions/currentdirectory.js
Outdated
dispatch(openSelectArtifactDirectoryDialog()); | ||
} else if (callback === 1) { | ||
curProps = ['openDirectory']; | ||
curTitle = 'Choose Artifact Directory'; |
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.
Choose Directory to Import
Just tested locally, I really like using the message box to select the file-open widget! |
app/js/actions/currentdirectory.js
Outdated
type: 'question', | ||
buttons: ['File', 'Directory', 'Cancel'], | ||
title: 'Artifact Selection', | ||
message: 'What would you like to select?', |
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.
Sorry one last comment. Change the message to: Are you importing a single file or directory?
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.
Sounds great on all fronts, thanks, will do!
In response to issue 107.
Functionality remains the same on mac os:
------------------------
------------------------
------------------------
However, Linux and Windows users should now have the ability to choose to either select a file or a directory. Previously they could only select directories, since on both OS's file choosers must be one or the other and default to directory when both options are passed. Demo on the latest Ubuntu in VirtualBox:
First, the user clicks
Import
. A dialog occurs asking if they would like to select a directory or a file (or cancel altogether).The user selects File.
The file is successfully imported.
Next, the user clicks
Import
again, and this time chooses directory. They navigate to the same directory as before, but this time they can't click on the file, only the directory, as below:(Notice that the titles on the choose windows change to say Choose Artifact Directory or Choose Artifact File contextually). The user clicks on the directory, fills in the parameters and clicks Go!. Success:
Untested on Windows, but it should work according to all the available documentation. (We don't yet support windows so this shouldn't matter too much for now anyway).