Student Submission Forms #142
Conversation
| setFieldValue(name, this.props.previewFile.path) | ||
| }) | ||
| break | ||
| default: |
There was a problem hiding this comment.
what are we doing about the video case?
There was a problem hiding this comment.
Students who want to submit a YouTube or Vimeo video will choose the "Video" Submission Form – this is the "Other Media" form where students can submit an image or PDF of something like a book or physical object.
We don't need to worry about videos on this form, as students cannot upload video files.
billdybas
force-pushed
the
forms
branch
2 times, most recently
from
46339e1
to
729dbad
Compare
And add skeletons for Video & Other Submission Forms
- Update Wording from Sponsor Feedback - Make 'moreCopies' dynamic based on 'forSale' value
We could add a 'Cancel' button to the Submission Chooser page, but it makes the page look a bit off - not exactly that aesthetically pleasing. Since you can click on the Nav Bar to get back to the homepage, I think it's fine to not add another button.
Also use correct mutation for Other Media Submission
| }) | ||
| break | ||
| default: | ||
| console.error(`Unknown File Type: ${file.type}`) |
There was a problem hiding this comment.
Can you put a // TODO handle error here, or somehow respond to errors?
There was a problem hiding this comment.
So, this code should technically never be reached. We only accept='application/pdf,image/jpeg', so there would need to be issues w/ the Dropzone or someone would have to be modifying our internals through dev tools.
There was a problem hiding this comment.
I remember I also added this case because in react-dropzone's docs, they mention that MIME type determination isn't reliable across platforms – if and end user is having issues uploading a file, someone would be able to diagnose the issue by having the end user open their console and see what the error message is.
See the accept prop: https://react-dropzone.js.org/
| </Container> | ||
| ) | ||
|
|
||
| export default Submit | ||
| const mapStateToProps = (state) => ({ | ||
| forShow: { id: 1, name: 'Honors Show 2017' } // TODO: Get from Redux |
There was a problem hiding this comment.
This is intentional? It's a pretty big TODO, we should at least open an issue for that
There was a problem hiding this comment.
Yes, this is intentional – not really a big issue. When @chtinahow has the Student dashboard complete, when you click "Make Submission" on a particular show, we'll either pass that show's id and name as props or access them through Redux. Since she'll have already queried for the show to display it on the dashboard, we'll just need to pass that data along.
If you want to make an Issue for integrating our two features together, feel free.
|
|
||
| const SuccessModal = (props) => ( | ||
| <Modal | ||
| isOpen={props.isOpen} |
There was a problem hiding this comment.
I guess this wold be for cleanliness sake, but why not break the styles out into their own divs?
There was a problem hiding this comment.
iirc, this was a bit hacky just to get the styles to work like I wanted. I couldn't use reacstrap's <ModalHeader /> component because the HTML it generates made it impossible to center the check icon.
If by "break[ing] the styles out into their own divs" you mean make them styled-components, I don't see any benefit over how the styles are right now – there's no component re-use. If we need more modals like this in the future, I could see making this a generic component and passing in props like the background color, icon, and body copy.
|
I think overall this is solid. The others covered any concern I have. So lgtm. |
Adds Student Submission Forms to the Student app.
Still To Do:
moreCopieson Video and Other Media submissionsmediaTypeon Photo Submission a react-selectCreatableredux-formtoformik(Probably a separate PR)~~~ => See Tech Debt: Finish Converting Forms to Formik #156