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

jujulcrane · 2024-12-14T15:10:13Z

Summary

Created a "You Need to Save!" modal if user tries to navigate away from a candidate without saving in the candidate decider. It allows them to either cancel, discard and proceed, or save and proceed.

Notion/Figma Link

https://www.notion.so/Candidate-Decider-Show-a-You-Need-to-Save-Modal-if-User-Tries-to-Navigate-Away-from-Candidate-W-1440ad723ce1806caae2c09118cacceb

Test Plan

Screen.Recording.2024-12-14.at.10.05.12.AM.mov

dti-github-bot · 2024-12-14T15:10:27Z

[diff-counting] Significant lines: 85.

andrew032011 · 2024-12-14T15:14:51Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

    }
  };

+  useEffect(() => {
+    setIsSaved(currentComment === defaultCurrentComment && currentRating === defaultCurrentRating);


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

andrew032011 · 2024-12-14T16:24:33Z

Can we also handle the two other methods of navigation? (Candidate ID dropdown and search bar)

andrew032011 · 2024-12-14T16:24:43Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

+          <p>You have unsaved changes. Do you want to save them before navigating?</p>
+        </Modal.Content>
+        <Modal.Actions>
+          <Button onClick={() => setIsOpen(false)}>Clancel</Button>


Suggested change

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

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

…ed state

andrew032011 · 2024-12-17T01:14:20Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

      <div className={styles.applicationContainer}>
        <div className={styles.searchBar}>
          <SearchBar
            instance={instance}
            setCurrentCandidate={(candidate) => {
-              setCurrentCandidate(candidate);
-              populateReviewForCandidate(candidate);
+              if (!isSaved) {


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.

andrew032011 · 2024-12-17T01:28:20Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

+              confirmNavigation(navigationDirection!);
+            }}
+          >
+            Save and {navigationDirection === 'previous' ? 'Go Back' : 'Proceed'}


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

andrew032011 · 2024-12-17T01:31:39Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

+  const confirmNavigation = (direction: 'next' | 'previous' | 'other') => {
+    setIsOpen(false);
+    setIsBypassingModal(true);
+    setTimeout(() => {


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

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

andrew032011

It's really coming together! Just some comments to help reduce code duplication

andrew032011 · 2024-12-19T03:10:44Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

@@ -17,6 +17,14 @@ type CandidateDeciderProps = {
 };

 const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
+  const [isOpen, setIsOpen] = useState(false);


rename to isModalOpen

andrew032011 · 2024-12-19T03:27:08Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

@@ -17,6 +17,14 @@ type CandidateDeciderProps = {
 };

 const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
+  const [isOpen, setIsOpen] = useState(false);
+  const [isBypassingModal, setIsBypassingModal] = useState(false);


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

andrew032011 · 2024-12-19T03:27:47Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

@@ -109,17 +145,51 @@ const CandidateDecider: React.FC<CandidateDeciderProps> = ({ uuid }) => {
    }
  };

+  const handleCandiateChange = (candidate: number) => {


Suggested change

const handleCandiateChange = (candidate: number) => {

const handleCandidateChange = (candidate: number) => {

andrew032011 · 2024-12-19T03:30:52Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

@@ -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) {


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 !

andrew032011 · 2024-12-19T03:34:45Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

+  const [navigationDirection, setNavigationDirection] = useState<
+    'next' | 'previous' | 'other' | null
+  >(null);
+  const [navigationDirectionCandidate, setNavigationDirectionCandidate] = useState<number | null>(


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

andrew032011 · 2024-12-22T17:46:31Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

+                currentRating ?? 0,
+                currentComment ?? ''
+              );
+              if (nextCandidate !== null) {


How about a function navigateToNextCandidate for this and the code that's being duplicated below?

andrew032011 · 2024-12-22T17:47:55Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

      <div className={styles.applicationContainer}>
        <div className={styles.searchBar}>
          <SearchBar
            instance={instance}
            setCurrentCandidate={(candidate) => {
-              setCurrentCandidate(candidate);
-              populateReviewForCandidate(candidate);
+              handleCandidateChange(candidate);


Actually you can just pass in handleCandidateChange here
i.e.

(candidate) => { handleCandidateChange(candidate); }

is the same as

handleCandidateChange

andrew032011 · 2024-12-22T17:50:08Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

+              disabled={currentCandidate === 0}
+              onClick={() => {
+                if (currentCandidate === 0) return;
+                setNextCandidate(currentCandidate - 1);


How about let's move setNextCandidate into handleCandidateChange? And move the bound checks there as well (==0 and == instance.candidates.length - 1)?

andrew032011 · 2024-12-22T17:50:37Z

frontend/src/components/Candidate-Decider/CandidateDecider.tsx

+            primary
+            onClick={() => {
+              if (nextCandidate !== null) {
+                setCurrentCandidate(nextCandidate);


See above, move this into a separate function navigateToNextCandidate to reduce code duplication

Created a "You need to save!" reminder on the Candidate Decider

732d47b

jujulcrane requested a review from a team as a code owner December 14, 2024 15:10

updated format and lint

8ff28ff

andrew032011 reviewed Dec 14, 2024

View reviewed changes

jujulcrane added 2 commits December 14, 2024 14:18

Added navigation for the search bar and id dropdown and removed isSav…

09278ea

…ed state

fixed format and lint

3d4f847

andrew032011 reviewed Dec 17, 2024

View reviewed changes

Update CandidateDecider.tsx

1129501

andrew032011 reviewed Dec 19, 2024

View reviewed changes

Update CandidateDecider.tsx

d4c18e7

andrew032011 reviewed Dec 22, 2024

View reviewed changes

reducing code duplication

ee07bdd

andrew032011 approved these changes Dec 26, 2024

View reviewed changes

jujulcrane merged commit c4d5c04 into main Dec 26, 2024
17 checks passed

jujulcrane deleted the juju-you-need-to-save branch December 26, 2024 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jujulcrane commented Dec 14, 2024

dti-github-bot commented Dec 14, 2024 •

edited

Loading

andrew032011 Dec 14, 2024

andrew032011 commented Dec 14, 2024

andrew032011 Dec 14, 2024

andrew032011 Dec 17, 2024

andrew032011 Dec 17, 2024

andrew032011 Dec 17, 2024

jujulcrane Dec 18, 2024

andrew032011 left a comment

andrew032011 Dec 19, 2024

andrew032011 Dec 19, 2024

andrew032011 Dec 19, 2024

andrew032011 Dec 19, 2024

andrew032011 Dec 19, 2024

andrew032011 Dec 22, 2024

andrew032011 Dec 22, 2024

andrew032011 Dec 22, 2024

andrew032011 Dec 22, 2024

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

	const handleCandiateChange = (candidate: number) => {
	const handleCandidateChange = (candidate: number) => {

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

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

Conversation

jujulcrane commented Dec 14, 2024

Summary

Notion/Figma Link

Test Plan

dti-github-bot commented Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

andrew032011 commented Dec 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew032011 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dti-github-bot commented Dec 14, 2024 •

edited

Loading