Code eval tool: use toast upon deletion instead of alert #10165

srietkerk · 2024-09-09T18:59:45Z

I found the alert experience to be really clunky, so, as per @eanders-ms's suggestion, I've changed it so when a user deletes a criteria, they can undo it via a toast that pops up.

As part of this change, I made it so deleting a criteria just performs a soft deletion so undoing a deletion was possible.

UPDATED, latest upload target (with export working correctly): https://makecode.microbit.org/app/9b9448bb0aa4de66361394d15e8a90bb8c7b1ac5-d565522e6f--eval

…etion

eanders-ms · 2024-09-09T19:45:01Z

When I delete a criteria, if that criteria was at max (and therefore not able to be added), it doesn't become available again. To repro:

Add "uses at least variables" to your checklist
Once added, delete that criteria from your checklist
Try to add it back again

It shows as Max in the catalog, and cannot be added.

teachertool/src/transforms/readdCriteriaToChecklist.ts

eanders-ms · 2024-09-09T20:06:17Z

teachertool/src/transforms/readdCriteriaToChecklist.ts

+import { Ticks } from "../constants";
+import { getCriteriaInstanceWithId } from "../state/helpers";
+
+export function readdCriteriaToChecklist(criteriaInstanceId: string) {


Over in addCriteriaToChecklist do you want to look for an existing deleted matching criteria and restore it if found? Would that make the distinction between adding and re-adding unnecessary?

An alternative might be to permanently delete the criteria instance once the toast disappears.

I think permanently deleting is probably for the best with this approach. Though see #10165 (comment) for some thoughts on a slightly different approach that I think would lessen the importance of it (would still ultimately be good to have, though).

Just for clarity, we are now permanently deleting the criteria after the toast goes away. I also wanted to add since this is related to the max problem that @eanders-ms expressed above, I removed the check for whether a criteria is deleted on the CatalogOverlay. I did so because having the check would allow a user to add and also readd a criteria, thus possibly having two instances of a criteria that is supposed to only have one. Getting rid of the check in the overlay makes it so it still looks like I can't add the "one only" criteria until the criteria is permanently deleted, thus avoiding the ability for a user to add and readd a criteria that can only be used once.

Co-authored-by: Eric Anderson <[email protected]>

…om/microsoft/pxt into srietkerk/tt-delete-criteria-toast

thsparks · 2024-09-10T17:01:43Z

teachertool/src/transforms/readdCriteriaToChecklist.ts

+import { Ticks } from "../constants";
+import { getCriteriaInstanceWithId } from "../state/helpers";
+
+export function readdCriteriaToChecklist(criteriaInstanceId: string) {


nit: I saw this as a typo of "read" instead of "re-add". Perhaps "reAdd" or "undelete" would be clearer?

thsparks · 2024-09-10T17:05:40Z

teachertool/src/components/CriteriaResultEntry.tsx

-        }
+        removeCriteriaFromChecklist(criteriaId);
+        const toast = makeToast("info", Strings.CriteriaDeleted);
+        toast.jsx = <UndoDeleteCriteriaButton criteriaId={criteriaId} toastId={toast.id} />;


As much as I like the sleekness of this design, something to keep in mind is from an accessibility perspective, how can a user navigate to this button without a mouse? If this means we need to treat toasts like modals and have them steal focus, that could be somewhat confusing/irritating. Maybe we could do that only if the toast is "interactive" in some way? Or there may be other workarounds.

Not necessarily a blocker, but good to keep it in mind.

Good point. I wonder what Outlook does here.

thsparks · 2024-09-10T17:07:56Z

teachertool/src/transforms/readdCriteriaToChecklist.ts

+    setChecklist(newChecklist);
+
+    if (catalogCriteriaId) {
+        pxt.tickEvent(Ticks.RemoveCriteria, { catalogCriteriaId });


Is removeCriteria the correct tick to send in this case?

thsparks · 2024-09-10T17:12:09Z

teachertool/src/transforms/removeCriteriaFromChecklist.ts


    const newChecklist = {
        ...teacherTool.checklist,
-        criteria: teacherTool.checklist.criteria.filter(c => c.instanceId !== criteriaInstanceId),
+        criteria: allCriteria,


So since we're just setting this flag but never removing the criteria, it this means that when a teacher exports their checklist, it will contain all the deleted checklist items which is somewhat wasteful since there's no way to get them back after the toast is dismissed.

One way to avoid this, which I think would ultimately be simpler all around, would be to move criteria out of the main checklist and into a separate deletedCriteria list, rather than add a delete flag. This means that, even if they export while the toast is open, the export will only contain the un-deleted criteria. It also means we don't need all the deleted checks within the code (which would be easy to forget when implementing future features).

Also, if we do a separate list, it's probably still good to remove items once the toast is dismissed, but not quite as important (assuming it clears when the user closes the session).

A downside to a separate list for deleted items is that undoing the deletion wouldn't preserve original location in the active criteria list (unless we stored the original index). If undeletion appended to the active set, it may confuse the user, especially if the end of the list is off the page.

Hmm, that's true, but I still think it'd be simpler to store a separate list of { criteria + index } objects than have to filter based on deleted everywhere (or remember to call the appropriate helper function)

A helper to get active criteria should mitigate this. I agree we don't want to include deleted criteria in the exported JSON.

Even with a helper, you have to remember to call it instead of getting the value from state :) Separate list makes it a non-issue as long as we can sort out the index thing. I don't feel that strongly, so I'm happy for Sarah to decide. But personally, I still think a separate list would be slightly cleaner.

I would like to continue with the flag approach and use the "get active criteria" helper function. I have been thinking A LOT about the pros and cons of each approach, and I think both have really strong merit. The selling point for me is for the removing and readding scenario of criteria. Both approaches have the same amount of state operations for full removing and newly adding criteria. However, in the new scenario that we are supporting here (removing and readding), the "having a separate list for deleted criteria" would need four state operations along with list operations (two for removing, two for readding) while the flag approach only needs two state operations and no list operations are needed.

That's fine, as long as we can get the helper in place and filter deleted criteria out of the serialization.

eanders-ms · 2024-09-10T17:28:44Z

teachertool/src/components/CatalogOverlay.tsx

@@ -80,7 +80,7 @@ const CatalogItem: React.FC<CatalogItemProps> = ({ catalogCriteria, recentlyAdde
    const { state: teacherTool } = useContext(AppStateContext);

    const existingInstanceCount = teacherTool.checklist.criteria.filter(
-        i => i.catalogCriteriaId === catalogCriteria.id
+        i => i.catalogCriteriaId === catalogCriteria.id && !i.deleted


For long-term code maintainability, I recommend adding a helper (in state/helpers.ts) for getting active criteria.

eanders-ms · 2024-09-10T17:30:03Z

teachertool/src/components/EvalResultDisplay.tsx

@@ -122,6 +122,7 @@ const CriteriaWithResultsTable: React.FC = () => {
        <div className={css["results-list"]}>
            {Object.values(criteria).map(criteriaInstance => {


For code clarity: Use helper to get active criteria, then render them.

thsparks · 2024-09-10T18:03:01Z

teachertool/src/style.overrides/Button.scss

+    flex-direction: column;
+    align-items: center;
+    justify-content: center;
+    background-color: var(--pxt-info-accent-darkened);


We should probably change these colors based on the type of toast containing them (error, info, etc...). I believe there are different css classes for each type of toast we can reference.

Yeah, I think that's a good idea. Can this be done in a follow-up?

thsparks · 2024-09-10T18:11:12Z

teachertool/src/components/CriteriaResultEntry.tsx

-        }
+        removeCriteriaFromChecklist(criteriaId);
+        const toast = makeToast("info", Strings.CriteriaDeleted);
+        toast.jsx = <UndoDeleteCriteriaButton criteriaId={criteriaId} toastId={toast.id} />;


In makeToast, it looks like you added a parameter for jsx, but it seems like we're not using it here. Was there a specific reason for that?

Yes. I originally just had the <UndoDeleteCriteriaButton/> inside the make toast. However, in order to have clicking the undo button dismiss the toast, I needed to have access to the toast id so I switched to setting the toast's jsx to the button after making the toast. That said, I could see future scenarios where passing in jsx when making the toast would be helpful so I left it in. I can remove it, though, since it's not getting used with this change.

I see. I would probably lean towards removing it in that case, at least for now. Either way is fine though.

…ction to helpers

thsparks · 2024-09-11T00:14:59Z

teachertool/src/transforms/removeCriteriaFromChecklist.ts

-        criteria: teacherTool.checklist.criteria.filter(c => c.instanceId !== criteriaInstanceId),
+        criteria: teacherTool.checklist.criteria.filter(c =>
+            c.instanceId !== criteriaInstanceId ||
+            c.instanceId === criteriaInstanceId && !c.deleted


Hmm, I'm wondering if there's a scenario where we would want this action to work even for non-deleted criteria (in which case we could have a separate action removeDeletedCriteriaFromChecklist that checks for deleted before running remove, or just an onlyIfDeleted parameter passed into this action). But I can't think of a realistic scenario at the moment so maybe it's fine...

thsparks · 2024-09-11T00:18:09Z

teachertool/src/transforms/removeCriteriaFromChecklist.ts

-        criteria: teacherTool.checklist.criteria.filter(c => c.instanceId !== criteriaInstanceId),
+        criteria: teacherTool.checklist.criteria.filter(c =>
+            c.instanceId !== criteriaInstanceId ||
+            c.instanceId === criteriaInstanceId && !c.deleted


I think the c.instanceId === criteriaInstanceId check is redundant. Could just be c.instanceId !== criteriaInstanceId || !c.deleted

thsparks · 2024-09-11T00:24:45Z

teachertool/src/types/criteria.ts

@@ -20,6 +20,7 @@ export interface CriteriaInstance {
    instanceId: string;
    params: CriteriaParameterValue[] | undefined;
    userFeedback?: UserFeedback;
+    deleted?: boolean;


I think we should still ensure deleted criteria get filtered out of export/serialization. It's not as big of an issue since it gets removed once the toast goes away, but the user could still export while the toast is around (or more likely, close the browser/tab, so it gets serialized and stored in browser storage), at which point the deleted criteria would be there forever since the delete code won't run on it anymore.

thsparks · 2024-09-11T00:25:41Z

teachertool/src/transforms/reAddCriteriaToChecklist.ts

+    setChecklist(newChecklist);
+
+    if (catalogCriteriaId) {
+        pxt.tickEvent(Ticks.RemoveCriteria, { catalogCriteriaId });


My original comment on this is out of date now, but just a note that I think we need to update this tick from Remove to Re-Add.

eanders-ms · 2024-09-11T03:18:43Z

When I open the Add Criteria panel and add a singleton criteria (limited to a single instance), it correctly appears the catalog unselectable and tagged with "Max". But after deleting the criteria, it remains unselectable in the catalog until the Undo toast disappears. This behavior seems incorrect. The catalog entry should immediately become available to add again.

eanders-ms · 2024-09-11T15:53:15Z

I think that soft deletion is complicating things here. Rather than soft delete, if the undo state were to hold onto the deleted criteria (alongside the toast id), it could simply re-add it at its original index when the user clicks Undo. Validation would need to run in the addCriteria transform to enforce constraints (Max instances, etc.). This is to handle the scenario where the user adds a new copy of the criteria before triggering Undo. Removing soft delete in favor of holding the deleted criteria in memory for a short time would simplify things a lot.

srietkerk added 5 commits September 5, 2024 16:56

soft deleting criteria entries to readd them if a user undoes the del…

1cb2d56

…etion

initial styling for undo button

1d1e11c

styling fixed for button

66ea6a2

don't evaluate deleted criteria

641a31f

undo will now dismiss the toast

98a0d0f

srietkerk requested a review from a team September 9, 2024 18:59

eanders-ms reviewed Sep 9, 2024

View reviewed changes

srietkerk and others added 3 commits September 10, 2024 09:45

Have log debug have the correct string on readd

275732b

Co-authored-by: Eric Anderson <[email protected]>

fix max with soft deletion

7f5298d

Merge branch 'srietkerk/tt-delete-criteria-toast' of https://github.c…

10f135f

…om/microsoft/pxt into srietkerk/tt-delete-criteria-toast

thsparks reviewed Sep 10, 2024

View reviewed changes

eanders-ms reviewed Sep 10, 2024

View reviewed changes

thsparks reviewed Sep 10, 2024

View reviewed changes

srietkerk added 5 commits September 10, 2024 16:18

hard delete criteria after toast goes away. added active criteria fun…

7cf2545

…ction to helpers

remove jsx from make toast since not being used

02d42a8

renamed readd to reAdd

0935b20

using helper for active criteria where necessary

6a0d7b5

only export active criteria

5031335

thsparks reviewed Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code eval tool: use toast upon deletion instead of alert #10165

Code eval tool: use toast upon deletion instead of alert #10165

srietkerk commented Sep 9, 2024 •

edited

Loading

eanders-ms commented Sep 9, 2024

eanders-ms Sep 9, 2024

eanders-ms Sep 9, 2024

thsparks Sep 10, 2024

srietkerk Sep 11, 2024

thsparks Sep 10, 2024

thsparks Sep 10, 2024

eanders-ms Sep 10, 2024

thsparks Sep 10, 2024

thsparks Sep 10, 2024 •

edited

Loading

thsparks Sep 10, 2024

eanders-ms Sep 10, 2024

thsparks Sep 10, 2024

eanders-ms Sep 10, 2024

thsparks Sep 10, 2024 •

edited

Loading

srietkerk Sep 10, 2024

thsparks Sep 10, 2024

eanders-ms Sep 10, 2024

eanders-ms Sep 10, 2024

thsparks Sep 10, 2024 •

edited

Loading

srietkerk Sep 10, 2024

thsparks Sep 11, 2024

thsparks Sep 10, 2024

srietkerk Sep 10, 2024

thsparks Sep 10, 2024

thsparks Sep 11, 2024

thsparks Sep 11, 2024

thsparks Sep 11, 2024

thsparks Sep 11, 2024

eanders-ms commented Sep 11, 2024

eanders-ms commented Sep 11, 2024

		@@ -122,6 +122,7 @@ const CriteriaWithResultsTable: React.FC = () => {
		<div className={css["results-list"]}>
		{Object.values(criteria).map(criteriaInstance => {

Code eval tool: use toast upon deletion instead of alert #10165

Are you sure you want to change the base?

Code eval tool: use toast upon deletion instead of alert #10165

Conversation

srietkerk commented Sep 9, 2024 • edited Loading

eanders-ms commented Sep 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thsparks Sep 10, 2024 • edited Loading

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

thsparks Sep 10, 2024 • edited Loading

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

thsparks Sep 10, 2024 • edited Loading

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

eanders-ms commented Sep 11, 2024

eanders-ms commented Sep 11, 2024

srietkerk commented Sep 9, 2024 •

edited

Loading

thsparks Sep 10, 2024 •

edited

Loading

thsparks Sep 10, 2024 •

edited

Loading

thsparks Sep 10, 2024 •

edited

Loading