-
Notifications
You must be signed in to change notification settings - Fork 2
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
40 retrieve config files from GitHub to display as default #46
Changes from 4 commits
a138583
c01da95
178d05b
8f7cdce
d5d2140
3fc39dc
bb95f78
45f21e5
44c593a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,11 @@ | |
<div class="container" style="width:100%"> | ||
<section id="upload3" class="upload" > | ||
<p>Optional Config File:</p> | ||
<input style="float:left" type="file" value="catib.config" name="configFile" id="configFile"/> | ||
<select id="config_file_selector"> | ||
<option>Choose File</option> | ||
<option>Upload your config file</option> | ||
</select> | ||
<input style="float:left; display:none" type="file" name="configFile" id="configFile"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When defining IDs sometimes camel case is used, while other times snake case is used. Please be consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will consistently define ids in camel case (as is with the original code) |
||
</section> | ||
</div> | ||
|
||
|
@@ -150,6 +154,74 @@ | |
<script src="scripts/saveSvgAsPng.js"></script> | ||
<script src="scripts/underscore-min.js"></script> | ||
<script src="scripts/cycle.js"></script> | ||
<script> | ||
var defaultConfigFiles = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please move this to a JS file (preferably a new file). |
||
|
||
function populateConfigFileSelectorHelper(files) { | ||
let option; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add comments explaining what each of these functions does? |
||
let selectList = document.getElementById("config_file_selector"); | ||
let lastOption = selectList.lastElementChild; | ||
// pop last child element of selectList | ||
selectList.removeChild(lastOption); | ||
for (let file of files) { | ||
option = document.createElement("option"); | ||
option.text = file.name; | ||
selectList.appendChild(option); | ||
}; | ||
selectList.appendChild(lastOption); | ||
// set the default value to the first option | ||
selectList.selectedIndex = 0; | ||
} | ||
|
||
function retrieveConfigFiles() { | ||
return new Promise((resolve, reject) => { | ||
let fileObjects = []; | ||
let rsp; | ||
sendRequest('GET', 'https://api.github.com/repos/CAMeL-Lab/palmyra/contents/palmyraSampleFiles/config').then(async rsp => { | ||
let files = JSON.parse(rsp); | ||
for (let file of files) { | ||
rsp = await sendRequest('GET', file.download_url); | ||
let fileObject = new File([rsp], file.name, {type: "text/plain"}); | ||
fileObjects.push(fileObject); | ||
} | ||
resolve(fileObjects); | ||
}).catch(err => { | ||
reject(err); | ||
}); | ||
}) | ||
} | ||
|
||
function populateConfigFileSelector() { | ||
let selectList = document.getElementById("config_file_selector"); | ||
selectList.onchange = () => { | ||
if (selectList.selectedIndex == selectList.options.length - 1) document.getElementById("configFile").style.display = "block"; | ||
else document.getElementById("configFile").style.display = "none" | ||
} | ||
retrieveConfigFiles().then(files => { | ||
defaultConfigFiles = files; | ||
populateConfigFileSelectorHelper(files); | ||
}).catch(err => { | ||
console.log(err); | ||
}); | ||
} | ||
|
||
function sendRequest(method, url, headerName="", headerValue="") { | ||
return new Promise((resolve, reject) => { | ||
let xhr = new XMLHttpRequest(); | ||
xhr.open(method, url); | ||
if (headerName !== "" && headerValue !== "") xhr.setRequestHeader(headerName, headerValue); | ||
xhr.onload = () => { | ||
if (xhr.status === 200) { | ||
resolve(xhr.responseText); | ||
} | ||
else { | ||
reject(xhr.statusText); | ||
} | ||
}; | ||
xhr.send(); | ||
}); | ||
} | ||
</script> | ||
<script src="main.js"></script> | ||
|
||
<script> | ||
|
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.
can you add a comment explaining what this does?
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.
The condition check
if (!alreadyReadConfigFiles.includes(file)){}
is from the original code. I guess the purpose is to only read each config file once. However, this does not seem necessary at the moment since we can only read maximum of 1 config file in the upload screen before going to the edit screen. When we want to go back to upload screen again to upload another config file, the window reloads andalreadyReadConfigFiles
array is reset to empty.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 mean add a comment in the code, so that when someone wants to go over the code, instead of unpacking code blocks, they read comments.