-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Reserve Form With Connection To Reserve API #1 #5 #6
Conversation
-Created App Component. -Created Reserve Component. -Created Release Component. -Added Material UI. -Added Redux. -Added Roboto font. -Created store. -Created actions. -Created reducer. -Added PDS app bar.
-Added the ability to upload and parse an excel sheet. -Added xlsx. -Parsed content is displayed on the reserve page.
-After an XLS file is uploaded and parsed the values are now shown in a table. -Date values are now kept as a string.
# Conflicts: # README.md
-Added form fields to enter reserve API parameters. -Used Saga to connect to reserve API.
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.
|
||
In the project directory, you can run: | ||
|
||
### `npm start` |
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 make the README a bit more specific.
Especially, we are missing the npm install
as a first step I think.
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.
Sure thing! This readme was auto generated by react but I can go through and make it more specific.
package.json
Outdated
@@ -0,0 +1,42 @@ | |||
{ | |||
"name": "doi-ui", |
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 (new) standard is to name the packages with pds- prefix, but have the repo name without the pds- prefix (the opposite of waht we usually do).
For now can you just rename the package pds-doi-ui.
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.
Got it, I'll make this change.
src/sagas/index.js
Outdated
let json = action.payload.excelContent; | ||
json = JSON.stringify(json); | ||
|
||
let endpoint = 'http://localhost:8085/PDS_APIs/pds_doi_api/0.1/dois'; |
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 would need to be in a configuration file somewhere
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'll move this out to a config.
Thank you for your feedback, I have only been doing minimal styling so far so I can take it into account. I have just been working on functionality not really the styling. I was thinking of doing the styling on a different issue as I wanted to use Mikes styling from the DOI and to make the forms look better. This takes some time though. I am not sure if we should include that in this pull request or after the app functionality is complete as it is simpler nearer the end. |
Thanks @eddiesarevalo for your feedback. The styling is not critical at all but the action menu should be removed and the reserve button should work (it did not in my deployment). Thanks |
-Removed action/draft drop down. This should always be set to draft. -Removed URL field. -Changed package name. -Moved out endpoint to a config.
-Updated the readme file with more details and build instructions.
-Fixed a typo in the last commit. The action should be reserve not draft.
@tloubrieu-jpl I have made the changes except for the editable table and the styling which will come later. When you attempted to run this locally were there any errors thrown on the developer console or network tab? |
The form for entering reserve API parameters including a parsed excel sheet has been added. The form includes a submit button that will send the parameters and parsed excel sheet to the reserve API.
Resolves #1
Resolves #5