Add file input dialog #90
Conversation
There was a problem hiding this comment.
@yoonieaj A general principle is to rely on the library to do the styling for us, and avoid custom CSS as much as possible. In this particular case, it seems like you have a bunch of custom CSS that's fighting against the default styling. I don't actually think that's necessary for this modal.
Use the provided components; it's fine to have the file input above the upload button rather than beside it. I imagine that's easier to achieve using these library components, and then the library and handle the layout for you.
| 2, | ||
| fileString | ||
| ); | ||
| expect(setTextDataMock).toHaveBeenCalledWith(fileString); |
There was a problem hiding this comment.
Previously, setTextData would be called called twice (once upon uploading the file, and again upon clicking the Load file data button). I made changes so that setTextData is only called once the button is clicked, and had to change this test to reflect that.
Pull Request Test Coverage Report for Build 11168684885Details
💛 - Coveralls |
demo/src/MemoryModelsUserInput.tsx
Outdated
| data-testid="file-input-modal" | ||
| > | ||
| <Paper | ||
| sx={{ |
There was a problem hiding this comment.
I'm not sure why any of these custom styles are necessary, doesn't the Modal component take care of the layout for us?
There was a problem hiding this comment.
The default styling for the modal was pretty much unusable:
As far as I can tell, the MUI Modal component doesn't actually have useful style presets. The examples in their documentation all set the CSS manually. (I did find something called Joy UI, which seems to be an offshoot of MUI that does offer good default Modal styling, but I think that's a separate library we'd have to install.)
There was a problem hiding this comment.
@yoonieaj aha, thanks for clarifying. This prompted me to do some more digging in the MUI documentation, since this seemed counter-intuitive to me. If you look at the documentation for Modal, it includes the following warning:
The term "modal" is sometimes used to mean "dialog", but this is a misnomer. A modal window describes parts of a UI. An element is considered modal if it blocks interaction with the rest of the application.
and
If you are creating a modal dialog, you probably want to use the Dialog component rather than directly using Modal. Modal is a lower-level construct that is leveraged by the following components...
So, I think we should be using Dialog here instead of Modal!
demo/src/MemoryModelsUserInput.tsx
Outdated
| Load file data | ||
| <div> | ||
| <Button onClick={handleOpen} sx={{ textTransform: "none" }}> | ||
| File Input |
There was a problem hiding this comment.
Rename to "Upload JSON File"
Proposed Changes
Adds a modal to the file input section to clean up the user interface.
...
current input section:
input section following changes:
The Upload JSON File button opens a modal:
Screenshots of your changes (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
Hello Professor! I managed to get the button to a reasonable size without having to rewrite any classes, but I can't seem to vertically center the file browse button within the modal window -- I included screenshots of this. From using the inspect tool, it seems like the file browse/input button is placed inside a div once the website is rendered but not centered within it. I'm not sure how to modify this div, or get rid of it. Do you have any suggestions on next steps? Thanks!