updates phases table to use mui confirmation modal #1467

tillyw · 2024-10-24T22:40:44Z

Associated issues

fixes cityofaustin/atd-data-tech#19436

Testing

URL to test:

https://deploy-preview-1467--atd-moped-main.netlify.app/

Steps to test:

go to a project's timeline tab and click the delete icon next to a phase
confirm that an mui confirmation modal appears asking 'Are you sure you want to delete this phase?'
click 'delete' and confirm the correct phase was deleted

Ship list

Code reviewed
Product manager approved
Product manager checked DB view dependencies
~~[ ] Product manager added to QA test script if applicable~~

mddilley

Works great - thanks for updating so these all work the same now! 🙌 🚢

mddilley · 2024-10-25T16:59:43Z

moped-editor/src/views/projects/projectView/ProjectPhases.js

@@ -133,7 +134,7 @@ const useColumns = ({ deleteInProgress, onDeletePhase, setEditPhase }) =>
                aria-label="delete"
                sx={{ color: "inherit" }}
                onClick={() =>
-                  onDeletePhase({ project_phase_id: row.project_phase_id })
+                  handleDeleteOpen({ project_phase_id: row.project_phase_id })


thank you for renaming this - very nice detail! 🙏 ✨

I like the handle prefix more than on but the DeleteOpen part is a little confusing to me. Maybe handleDeleteModalOpen would clarify it for me.

or it could read even nicer if it was openDeleteModal. That would make the code more readable to the actually story of, "on click of the delete icon, it opens the delete modal." I'm getting way too deep into the semantics.

roseeichelmann

test steps check out 🚀 i wonder if theres a way to fix the bug where the table resizes for a second on delete before going back? but not a big deal and this is good to go!

mateoclarke

🚢 Testest according to the steps and all looks good.

I do notice inconsistency within the two tables on the Timelime tab where the Milestones table has a row inline to confim delete while this Phases table uses the nice modal as implemented by Tilly. I assume bringing the Milestones one on board to the model UX is on @mddilley's radar and accounted for in the backlog.

mateoclarke · 2024-10-29T22:29:50Z

moped-editor/src/views/projects/projectView/ProjectPhases.js

@@ -133,7 +134,7 @@ const useColumns = ({ deleteInProgress, onDeletePhase, setEditPhase }) =>
                aria-label="delete"
                sx={{ color: "inherit" }}
                onClick={() =>
-                  onDeletePhase({ project_phase_id: row.project_phase_id })
+                  handleDeleteOpen({ project_phase_id: row.project_phase_id })


mateoclarke · 2024-10-29T22:43:44Z

moped-editor/src/views/projects/projectView/ProjectPhases.js

@@ -133,7 +134,7 @@ const useColumns = ({ deleteInProgress, onDeletePhase, setEditPhase }) =>
                aria-label="delete"
                sx={{ color: "inherit" }}
                onClick={() =>
-                  onDeletePhase({ project_phase_id: row.project_phase_id })
+                  handleDeleteOpen({ project_phase_id: row.project_phase_id })


I like the handle prefix more than on but the DeleteOpen part is a little confusing to me. Maybe handleDeleteModalOpen would clarify it for me.

mateoclarke · 2024-10-29T22:47:09Z

moped-editor/src/views/projects/projectView/ProjectPhases.js

@@ -133,7 +134,7 @@ const useColumns = ({ deleteInProgress, onDeletePhase, setEditPhase }) =>
                aria-label="delete"
                sx={{ color: "inherit" }}
                onClick={() =>
-                  onDeletePhase({ project_phase_id: row.project_phase_id })
+                  handleDeleteOpen({ project_phase_id: row.project_phase_id })


or it could read even nicer if it was openDeleteModal. That would make the code more readable to the actually story of, "on click of the delete icon, it opens the delete modal." I'm getting way too deep into the semantics.

mateoclarke · 2024-10-29T22:50:28Z

moped-editor/src/views/projects/projectView/ProjectPhases.js

+    setDeleteConfirmationId(project_phase_id);
+  }, []);
+
+  const handleDeleteClick = useCallback(


not sure what we call similar functions that use that pattern (I just checked and most of them are anonymous functions) but a nice name here could be confirmDelete. But I do see this same name used in ProjectFiles and Funding so being consistent with the pattern is probably best.

chiaberry

Nice to keep knocking out these UX improvements

Naming things is hard, I agree with Mateo that we could do a better job naming these (I'm afraid to check and see that it was me who named these functions). but this does follow the pattern we have everywhere else, so if we do rename this it should be across all our confirmation modals

tillyw · 2024-10-30T20:00:51Z

Nice to keep knocking out these UX improvements

Naming things is hard, I agree with Mateo that we could do a better job naming these (I'm afraid to check and see that it was me who named these functions). but this does follow the pattern we have everywhere else, so if we do rename this it should be across all our confirmation modals

thanks for pointing that out, @chiaberry! you're right, i was following the existing patterns and i totally agree about keeping it consistent. so i'm not sure if i should go ahead and rename these functions in other components in this pr or stick with the naming convention we have now and then perhaps we can come up with guidelines for a new naming convention and follow up with that as tech debt. @mddilley do you have any leanings?

mddilley · 2024-11-04T17:40:32Z

@tillyw Yeah, renaming them all sounds good to me to keep the code consistent. If you don't mind updating in this PR, then go for it! I like handleDeleteModalOpen that Mateo suggested above. I can also spin off an issue if you want to move on from this PR.

mddilley · 2024-11-07T16:44:44Z

I went ahead and created a snackoo for Mateo's suggestion about function naming earlier in the PR conversation. cityofaustin/atd-data-tech#19814

@tillyw This is good to merge!

tillyw added 3 commits October 24, 2024 17:39

updates phases table to use mui confirmation modal

482c5bd

update text

046ce1e

fix hook dependency

8682361

tillyw requested review from mddilley, chiaberry, frankhereford, mateoclarke, johnclary and roseeichelmann and removed request for mddilley October 24, 2024 22:53

mddilley approved these changes Oct 25, 2024

View reviewed changes

roseeichelmann approved these changes Oct 28, 2024

View reviewed changes

mateoclarke approved these changes Oct 29, 2024

View reviewed changes

chiaberry approved these changes Oct 30, 2024

View reviewed changes

mddilley mentioned this pull request Nov 7, 2024

Update DeleteConfirmationModal handler functions to have consistent and better names cityofaustin/atd-data-tech#19814

Open

mddilley merged commit 203e289 into main Nov 7, 2024
4 checks passed

mddilley deleted the 19436_update_phases_table_to_use_mui_confirmation_modal branch November 7, 2024 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates phases table to use mui confirmation modal #1467

updates phases table to use mui confirmation modal #1467

tillyw commented Oct 24, 2024 •

edited by mddilley

Loading

mddilley left a comment

mddilley Oct 25, 2024

mateoclarke Oct 29, 2024

mateoclarke Oct 29, 2024

mateoclarke Oct 29, 2024

roseeichelmann left a comment

mateoclarke left a comment

mateoclarke Oct 29, 2024

mateoclarke Oct 29, 2024

mateoclarke Oct 29, 2024

mateoclarke Oct 29, 2024

chiaberry left a comment

tillyw commented Oct 30, 2024

mddilley commented Nov 4, 2024

mddilley commented Nov 7, 2024

updates phases table to use mui confirmation modal #1467

updates phases table to use mui confirmation modal #1467

Conversation

tillyw commented Oct 24, 2024 • edited by mddilley Loading

Associated issues

Testing

Ship list

mddilley 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

roseeichelmann left a comment

Choose a reason for hiding this comment

mateoclarke 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

chiaberry left a comment

Choose a reason for hiding this comment

tillyw commented Oct 30, 2024

mddilley commented Nov 4, 2024

mddilley commented Nov 7, 2024

tillyw commented Oct 24, 2024 •

edited by mddilley

Loading