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.

Sign up for GitHub

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

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

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
732d47b
Created a "You need to save!" reminder on the Candidate Decider
jujulcrane Dec 14, 2024
8ff28ff
updated format and lint
jujulcrane Dec 14, 2024
09278ea
Added navigation for the search bar and id dropdown and removed isSav…
jujulcrane Dec 14, 2024
3d4f847
fixed format and lint
jujulcrane Dec 14, 2024
1129501
Update CandidateDecider.tsx
jujulcrane Dec 18, 2024
d4c18e7
Update CandidateDecider.tsx
jujulcrane Dec 20, 2024
ee07bdd
reducing code duplication
jujulcrane Dec 25, 2024
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
101 changes: 89 additions & 12 deletions frontend/src/components/Candidate-Decider/CandidateDecider.tsx
View file
Open in desktop
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,14 @@ type CandidateDeciderProps = {
};

const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
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' | 'other' | null
>(null);
const [navigationDirectionCandidate, setNavigationDirectionCandidate] = useState<number | null>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename? "navigationDirection" doesn't really apply anymore if we're not tracking navigation direction

null
);
const [currentCandidate, setCurrentCandidate] = useState<number>(0);
const [showOtherVotes, setShowOtherVotes] = useState<boolean>(false);

Expand Down Expand Up @@ -46,6 +54,9 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
const [defaultCurrentRating, setDefaultCurrentRating] = useState<Rating>();
const [defaultCurrentComment, setDefaultCurrentComment] = useState<string>();

const isSaved =
currentComment === defaultCurrentComment && currentRating === defaultCurrentRating;

const populateReviewForCandidate = (candidate: number) => {
const rating = getRating(candidate);
const comment = getComment(candidate);
Expand All @@ -68,7 +79,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 +94,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 +109,21 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
});
};

const confirmNavigation = (direction: 'next' | 'previous' | 'other') => {
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 if (direction === 'previous') previous(true);
else if (direction === 'other') {
if (navigationDirectionCandidate) {
setCurrentCandidate(navigationDirectionCandidate);
populateReviewForCandidate(navigationDirectionCandidate);
}
}
}, 0);
};

const handleRatingAndCommentChange = (id: number, rating: Rating, comment: string) => {
CandidateDeciderAPI.updateRatingAndComment(instance.uuid, id, rating, comment);
if (userInfo) {
Expand Down Expand Up @@ -113,13 +151,43 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
<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)}>Cancel</Button>
<Button
onClick={() => {
handleRatingAndCommentChange(
currentCandidate,
currentRating ?? 0,
currentComment ?? ''
);
confirmNavigation(navigationDirection!);
}}
>
Save and {navigationDirection === 'previous' ? 'Go Back' : 'Proceed'}
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 worried that this language is confusing "Go Back" might be interpreted as going back to the previous state (someone might think this is the same as "Discard")

maybe we don't need to show the navigation direction? Just "Save", "Discard", and "Cancel" seem good with me

</Button>
<Button primary onClick={() => confirmNavigation(navigationDirection!)}>
Discard and {navigationDirection === 'previous' ? 'Go Back' : 'Proceed'}
</Button>
</Modal.Actions>
</Modal>
<div className={styles.applicationContainer}>
<div className={styles.searchBar}>
<SearchBar
instance={instance}
setCurrentCandidate={(candidate) => {
setCurrentCandidate(candidate);
populateReviewForCandidate(candidate);
if (!isSaved) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you abstract this function and the below into a helper function? for cleanliness ideally we can put all the handling for this into a single function, as opposed to spreading the logic around.

setNavigationDirection('other');
setNavigationDirectionCandidate(candidate);
setIsOpen(true);
} else {
setCurrentCandidate(candidate);
populateReviewForCandidate(candidate);
}
}}
currentCandidate={currentCandidate}
/>
Expand All @@ -136,29 +204,38 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
text: candidate.id
}))}
onChange={(_, data) => {
setCurrentCandidate(data.value as number);
populateReviewForCandidate(data.value as number);
if (!isSaved) {
setNavigationDirection('other');
setNavigationDirectionCandidate(data.value as number);
setIsOpen(true);
} else {
setCurrentCandidate(data.value as number);
populateReviewForCandidate(data.value as number);
}
}}
/>
<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