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

Created a "You need to save!" Modal on the Candidate Decider #799

Merged
merged 7 commits into from
Dec 26, 2024
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
73 changes: 65 additions & 8 deletions frontend/src/components/Candidate-Decider/CandidateDecider.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useEffect, useState } from 'react';
import { Button, Dropdown, Checkbox } from 'semantic-ui-react';
import { Button, Dropdown, Checkbox, Modal } from 'semantic-ui-react';
import CandidateDeciderAPI from '../../API/CandidateDeciderAPI';
import ResponsesPanel from './ResponsesPanel';
import LocalProgressPanel from './LocalProgressPanel';
Expand All @@ -17,6 +17,10 @@ type CandidateDeciderProps = {
};

const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
const [isSaved, setIsSaved] = useState(true);
const [isOpen, setIsOpen] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to isModalOpen

const [isBypassingModal, setIsBypassingModal] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure we can get rid of this isBypassingModal entirely. It feels like a combination of force and isSaved is enough.

const [navigationDirection, setNavigationDirection] = useState<'next' | 'previous' | null>(null);
const [currentCandidate, setCurrentCandidate] = useState<number>(0);
const [showOtherVotes, setShowOtherVotes] = useState<boolean>(false);

Expand Down Expand Up @@ -68,7 +72,13 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [currentCandidate, instance.candidates, reviews]);

const next = () => {
const next = (force = false) => {
if (!force && !isSaved && !isBypassingModal) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that we're not using navigationDirection anymore for the text, it feels like prev, next, and handleCandidateChange can all be condensed into just one function. Can you try deleting prev and next and just using handleCandiateChange? (you might have to check if candidate number is within 0...instance.candidates.length - 1 but otherwise the looks the same to me)

and then we can get rid of navigationDirection !

setNavigationDirection('next');
setIsOpen(true);
return;
}
setIsBypassingModal(false);
if (currentCandidate === instance.candidates.length - 1) return;
setCurrentCandidate((prev) => {
const nextCandidate = prev + 1;
Expand All @@ -77,7 +87,13 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
});
};

const previous = () => {
const previous = (force = false) => {
if (!force && !isBypassingModal) {
setNavigationDirection('previous');
setIsOpen(true);
return;
}
setIsBypassingModal(false);
if (currentCandidate === 0) return;
setCurrentCandidate((prev) => {
const prevCandidate = prev - 1;
Expand All @@ -86,6 +102,15 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
});
};

const confirmNavigation = (direction: 'next' | 'previous') => {
setIsOpen(false);
setIsBypassingModal(true);
setTimeout(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, wondering what the point of setTimeout with 0 milliseconds is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh oops I forgot to get rid of that - I was trying to fix a problem I was running into where it was making the user click save twice before it worked but I ended up fixing it a different way

if (direction === 'next') next(true);
else previous(true);
}, 0);
};

const handleRatingAndCommentChange = (id: number, rating: Rating, comment: string) => {
CandidateDeciderAPI.updateRatingAndComment(instance.uuid, id, rating, comment);
if (userInfo) {
Expand All @@ -106,13 +131,42 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
uuid: ''
}
]);
setIsSaved(true);
}
};

useEffect(() => {
setIsSaved(currentComment === defaultCurrentComment && currentRating === defaultCurrentRating);
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of storing this value in a state, what if we just had a variable that stored it? currentComment, currentRating, defaultCurrentComment, defaultCurrentRating are all state variables so they'd trigger a re-render anyways when they're changed.

In general it isn't the best pattern to store state that's derived from other states, since it requires you to keep them in sync, and can get lead to buggy behavior if they ever became out of sync

}, [currentComment, currentRating, defaultCurrentComment, defaultCurrentRating]);

return instance.candidates.length === 0 ? (
<div></div>
) : (
<div className={styles.candidateDeciderContainer}>
<Modal open={isOpen} onClose={() => setIsOpen(false)} size="small">
<Modal.Header>Don't Forget To Save!</Modal.Header>
<Modal.Content>
<p>You have unsaved changes. Do you want to save them before navigating?</p>
</Modal.Content>
<Modal.Actions>
<Button onClick={() => setIsOpen(false)}>Clancel</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Button onClick={() => setIsOpen(false)}>Clancel</Button>
<Button onClick={() => setIsOpen(false)}>Cancel</Button>

<Button
onClick={() => {
handleRatingAndCommentChange(
currentCandidate,
currentRating ?? 0,
currentComment ?? ''
);
confirmNavigation(navigationDirection!);
}}
>
Save and {navigationDirection === 'next' ? 'Proceed' : 'Go Back'}
</Button>
<Button primary onClick={() => confirmNavigation(navigationDirection!)}>
Discard and {navigationDirection === 'next' ? 'Proceed' : 'Go Back'}
</Button>
</Modal.Actions>
</Modal>
<div className={styles.applicationContainer}>
<div className={styles.searchBar}>
<SearchBar
Expand Down Expand Up @@ -142,23 +196,26 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
/>
<span className={styles.ofNum}>of {instance.candidates.length}</span>
<Button.Group className={styles.previousNextButtonContainer}>
<Button basic color="blue" disabled={currentCandidate === 0} onClick={previous}>
<Button
basic
color="blue"
disabled={currentCandidate === 0}
onClick={() => previous(isSaved)}
>
PREVIOUS
</Button>
<Button
basic
color="blue"
disabled={currentCandidate === instance.candidates.length - 1}
onClick={next}
onClick={() => next(isSaved)}
>
NEXT
</Button>
</Button.Group>
<Button
className="ui blue button"
disabled={
currentComment === defaultCurrentComment && currentRating === defaultCurrentRating
}
disabled={isSaved}
onClick={() => {
handleRatingAndCommentChange(
currentCandidate,
Expand Down
Loading