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

feat: new UX for submitting changes #224

Merged
merged 8 commits into from
Jul 4, 2023
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Alert, Box, Button, Snackbar, Typography } from "@mui/material";
import { Alert, Box, Snackbar, Typography, Fab } from "@mui/material";
import SaveIcon from "@mui/icons-material/Save";
import { useEffect, useState } from "react";
import useFetch from "../../components/useFetch";
import ListEntryParents from "./ListEntryParents";
Expand Down Expand Up @@ -29,8 +30,19 @@ const AccumulateAllComponents = ({ id, taxonomyName, branchName }) => {
errorMessage,
} = useFetch(url);
const [nodeObject, setNodeObject] = useState(null); // Storing updates to node
const [originalNodeObject, setOriginalNodeObject] = useState(null); // For tracking changes
const [updateChildren, setUpdateChildren] = useState([]); // Storing updates of children in node
const [open, setOpen] = useState(false); // Used for Dialog component
const [changesMade, setChangesMade] = useState(false); // Used for displaying Fab
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably name this to isNewChange?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what about hasChanges


// Tracking changes
useEffect(() => {
if (nodeObject && originalNodeObject) {
const changesMade =
JSON.stringify(nodeObject) !== JSON.stringify(originalNodeObject);
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON.stringify does not ensure that the strings will have the same shape. Let's use https://www.npmjs.com/package/fast-deep-equal for this. Also, no need for this to be a state, it can be a derived value

setChangesMade(changesMade);
}
}, [nodeObject, originalNodeObject]);

// Setting state of node after fetch
useEffect(() => {
Expand All @@ -54,6 +66,7 @@ const AccumulateAllComponents = ({ id, taxonomyName, branchName }) => {
});
}
setNodeObject(duplicateNode);
setOriginalNodeObject(JSON.parse(JSON.stringify(duplicateNode))); // Deep copy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the parse and stringify?

}, [node]);

// Displaying error messages if any
Expand Down Expand Up @@ -145,14 +158,18 @@ const AccumulateAllComponents = ({ id, taxonomyName, branchName }) => {
/>{" "}
</>
)}
{/* Button for submitting edits */}
<Button
variant="contained"
onClick={handleSubmit}
sx={{ ml: 4, mt: 2, width: 130, mb: 2 }}
>
Submit
</Button>
{/* Fab for submitting edits */}
{changesMade && (
<Fab
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we add some background color for the Fab, just so that the button is not floating on top of the content?

variant="extended"
onClick={handleSubmit}
color="primary"
sx={{ position: "fixed", bottom: 16, left: 16 }}
>
<SaveIcon sx={{ mr: 1 }} />
Save Changes
</Fab>
)}
{/* Snackbar for acknowledgment of update */}
<Snackbar
anchorOrigin={{ vertical: "top", horizontal: "right" }}
Expand Down