Skip to content
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 file input dialog #90

Merged
merged 13 commits into from
Oct 3, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

- Made text input box use a monospace font
- Disabled download buttons when there is no input/output.
- Added a modal to the file input section.

### πŸ”§ Internal changes

Expand Down
73 changes: 53 additions & 20 deletions demo/src/MemoryModelsUserInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ import {
Tooltip,
MenuItem,
Stack,
Modal,
Paper,
Card,
CardContent,
} from "@mui/material";
import DownloadJSONButton from "./DownloadJSONButton";
import MemoryModelsMenu from "./MemoryModelsMenu";
Expand Down Expand Up @@ -45,6 +49,10 @@ type MemoryModelsUserInputPropTypes = MemoryModelsFileInputPropTypes &

function MemoryModelsFileInput(props: MemoryModelsFileInputPropTypes) {
const [uploadedFileString, setUploadedFileString] = useState("");
const [open, setOpen] = useState(false);

const handleOpen = () => setOpen(true);
const handleClose = () => setOpen(false);

const onChange = (event) => {
try {
Expand All @@ -54,7 +62,6 @@ function MemoryModelsFileInput(props: MemoryModelsFileInputPropTypes) {
fileReader.onload = (event) => {
const fileString = event.target.result as string;
setUploadedFileString(fileString);
props.setTextData(fileString);
};
} catch (error) {
const errorMessage = `Error reading uploaded file as text. Please ensure it's in UTF-8 encoding: ${error.message}`;
Expand All @@ -66,29 +73,55 @@ function MemoryModelsFileInput(props: MemoryModelsFileInputPropTypes) {

const onLoadButtonClick = () => {
props.setTextData(uploadedFileString);
setOpen(false);
};

return (
<Stack direction={"row"} spacing={2}>
<Input
type="file"
onChange={onChange}
inputProps={{
accept: "application/JSON",
"data-testid": "file-input",
}}
disableUnderline={true}
/>
<Button
data-testid="file-input-reapply-button"
variant="contained"
disabled={!uploadedFileString}
onClick={onLoadButtonClick}
sx={{ textTransform: "none" }}
>
Load file data
<div>
<Button onClick={handleOpen} sx={{ textTransform: "none" }}>
File Input
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to "Upload JSON File"

</Button>
</Stack>
<Modal
open={open}
onClose={handleClose}
data-testid="file-input-modal"
>
<Paper
sx={{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why any of these custom styles are necessary, doesn't the Modal component take care of the layout for us?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default styling for the modal was pretty much unusable:

image

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.)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

position: "absolute",
top: "40%",
left: "20%",
width: "50%",
padding: 2,
}}
>
<div>
<Input
type="file"
onChange={onChange}
inputProps={{
accept: "application/JSON",
"data-testid": "file-input",
}}
disableUnderline={true}
sx={{ alignSelf: "center" }}
/>
</div>
<div>
<Button
data-testid="file-input-reapply-button"
variant="contained"
color="primary"
disabled={!uploadedFileString}
onClick={onLoadButtonClick}
sx={{ textTransform: "none" }}
>
Load file data
</Button>
</div>
</Paper>
</Modal>
</div>
);
}

Expand Down
25 changes: 19 additions & 6 deletions demo/src/__tests__/MemoryModelsUserInput.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import React from "react";
import { fireEvent, render, screen, waitFor } from "@testing-library/react";
import {
fireEvent,
getByText,
render,
screen,
waitFor,
} from "@testing-library/react";
import MemoryModelsUserInput from "../MemoryModelsUserInput";

describe("MemoryModelsUserInput", () => {
Expand Down Expand Up @@ -127,7 +133,16 @@ describe("MemoryModelsUserInput", () => {
jest.restoreAllMocks();
});

it("does not render the modal when the page first loads", () => {
const modal = screen.queryByTestId("file-input-modal");
expect(modal).toBeNull();

const input: HTMLInputElement = screen.queryByTestId("file-input");
expect(input).toBeNull();
});

it("renders an enabled input and disabled reapply button", () => {
fireEvent.click(screen.getByText("File Input"));
const input: HTMLInputElement = screen.getByTestId("file-input");
expect(input).toHaveProperty("disabled", false);

Expand All @@ -149,6 +164,7 @@ describe("MemoryModelsUserInput", () => {
type: "application/json",
}
);
fireEvent.click(screen.getByText("File Input"));
const input: HTMLInputElement = screen.getByTestId("file-input");
await waitFor(() => {
// this needs to be awaited because of fileReader.onload being async
Expand All @@ -167,6 +183,7 @@ describe("MemoryModelsUserInput", () => {
let input: HTMLInputElement;

beforeEach(async () => {
fireEvent.click(screen.getByText("File Input"));
const file = new File([fileString], "test.json", {
type: "application/json",
});
Expand Down Expand Up @@ -195,13 +212,9 @@ describe("MemoryModelsUserInput", () => {
});

await waitFor(() => {
// once from reapplyBtn onChange, once from MemoryModelsTextInput handleTextFieldChange
// if put within the same waitFor block as fireEvent.click(reapplyBtn), this test always passes
// even with the wrong expect
expect(setTextDataMock).toHaveBeenNthCalledWith(
2,
fileString
);
expect(setTextDataMock).toHaveBeenCalledWith(fileString);
Copy link
Collaborator Author

@yoonieaj yoonieaj Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
});
});
Expand Down
Loading