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

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1cb2d56
soft deleting criteria entries to readd them if a user undoes the del…
srietkerk Sep 5, 2024
1d1e11c
initial styling for undo button
srietkerk Sep 6, 2024
66ea6a2
styling fixed for button
srietkerk Sep 6, 2024
641a31f
don't evaluate deleted criteria
srietkerk Sep 6, 2024
98a0d0f
undo will now dismiss the toast
srietkerk Sep 9, 2024
275732b
Have log debug have the correct string on readd
srietkerk Sep 10, 2024
7f5298d
fix max with soft deletion
srietkerk Sep 10, 2024
10f135f
Merge branch 'srietkerk/tt-delete-criteria-toast' of https://github.c…
srietkerk Sep 10, 2024
7cf2545
hard delete criteria after toast goes away. added active criteria fun…
srietkerk Sep 10, 2024
02d42a8
remove jsx from make toast since not being used
srietkerk Sep 10, 2024
0935b20
renamed readd to reAdd
srietkerk Sep 10, 2024
6a0d7b5
using helper for active criteria where necessary
srietkerk Sep 10, 2024
5031335
only export active criteria
srietkerk Sep 10, 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
2 changes: 1 addition & 1 deletion teachertool/src/components/CatalogOverlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

).length;
const isMaxed = catalogCriteria.maxCount !== undefined && existingInstanceCount >= catalogCriteria.maxCount;
return catalogCriteria.template ? (
Expand Down
28 changes: 25 additions & 3 deletions teachertool/src/components/CriteriaResultEntry.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import { Button } from "react-common/components/controls/Button";
import { setEvalResult } from "../transforms/setEvalResult";
import { showToast } from "../transforms/showToast";
import { makeToast } from "../utils";
import { readdCriteriaToChecklist } from "../transforms/readdCriteriaToChecklist";
import { dismissToast } from "../state/actions";

interface CriteriaResultNotesProps {
criteriaId: string;
Expand Down Expand Up @@ -76,6 +78,25 @@ const CriteriaResultError: React.FC<CriteriaResultErrorProps> = ({ criteriaInsta
);
};

const UndoDeleteCriteriaButton: React.FC<{ criteriaId: string, toastId: string }> = ({ criteriaId, toastId }) => {
const { dispatch } = useContext(AppStateContext);
const handleUndoClicked = () => {
readdCriteriaToChecklist(criteriaId);
if (toastId) {
dispatch(dismissToast(toastId));
}
}

return (
<Button
className="undo-button"
title={Strings.Undo}
label={Strings.Undo}
onClick={handleUndoClicked}
/>
)
}

const CriteriaResultToolbarTray: React.FC<{ criteriaId: string }> = ({ criteriaId }) => {
const { state: teacherTool } = useContext(AppStateContext);

Expand All @@ -91,9 +112,10 @@ const CriteriaResultToolbarTray: React.FC<{ criteriaId: string }> = ({ criteriaI
}

async function handleDeleteClickedAsync() {
if (confirm(Strings.ConfirmDeleteCriteriaInstance)) {
removeCriteriaFromChecklist(criteriaId);
}
removeCriteriaFromChecklist(criteriaId);
const toast = makeToast("info", Strings.CriteriaDeleted);
toast.jsx = <UndoDeleteCriteriaButton criteriaId={criteriaId} toastId={toast.id} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I wonder what Outlook does here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

showToast(toast);
}

return (
Expand Down
1 change: 1 addition & 0 deletions teachertool/src/components/EvalResultDisplay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ const CriteriaWithResultsTable: React.FC = () => {
<div className={css["results-list"]}>
{Object.values(criteria).map(criteriaInstance => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return (
!criteriaInstance.deleted &&
<CriteriaResultEntry criteriaId={criteriaInstance.instanceId} key={criteriaInstance.instanceId} />
);
})}
Expand Down
2 changes: 1 addition & 1 deletion teachertool/src/components/Toasts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const ToastNotification: React.FC<IToastNotificationProps> = ({ toast }) => {
<div className={css["text-container"]}>
{toast.text && <div className={classList(css["text"], "tt-toast-text")}>{toast.text}</div>}
{toast.detail && <div className={css["detail"]}>{toast.detail}</div>}
{toast.jsx && <div>{toast.jsx}</div>}
{toast.jsx && <div className={css["custom-jsx"]}>{toast.jsx}</div>}
</div>
{!toast.hideDismissBtn && !toast.showSpinner && (
<div className={css["dismiss-btn"]} onClick={handleDismissClicked}>
Expand Down
9 changes: 8 additions & 1 deletion teachertool/src/components/styling/Toasts.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
gap: 0.5rem;
padding: 0.75rem;
font-size: 1.125rem;
align-items: center;

.icon-container {
display: flex;
Expand Down Expand Up @@ -72,7 +73,8 @@

.text-container {
display: flex;
flex-direction: column;
flex-direction: row;
align-items: center;
text-align: left;

.text {
Expand All @@ -85,6 +87,11 @@
.detail {
font-size: 0.875rem;
}

.custom-jsx {
pointer-events: auto;
margin-left: 1rem;
}
}

.dismiss-btn {
Expand Down
2 changes: 2 additions & 0 deletions teachertool/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ export namespace Strings {
export const EvaluationComplete = lf("Evaluation complete");
export const UnableToEvaluatePartial = lf("Unable to evaluate some criteria");
export const GiveFeedback = lf("Give Feedback");
export const CriteriaDeleted = lf("Criteria Deleted");
export const Undo = lf("Undo");
}

export namespace Ticks {
Expand Down
11 changes: 11 additions & 0 deletions teachertool/src/style.overrides/Button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,14 @@
.pass > .common-button.common-dropdown-button {
border: 3px solid var(--pxt-success-accent);
}

.undo-button {
width: 3rem;
height: 2rem;
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
background-color: var(--pxt-info-accent-darkened);
Copy link
Contributor

@thsparks thsparks Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

color: var(--pxt-button-secondary-foreground);
}
28 changes: 28 additions & 0 deletions teachertool/src/transforms/readdCriteriaToChecklist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { stateAndDispatch } from "../state";
import { logDebug } from "../services/loggingService";
import { setChecklist } from "./setChecklist";
import { Ticks } from "../constants";
import { getCriteriaInstanceWithId } from "../state/helpers";

export function readdCriteriaToChecklist(criteriaInstanceId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const { state: teacherTool } = stateAndDispatch();

logDebug(`Re-adding criteria with id: ${criteriaInstanceId}`);

const instance = getCriteriaInstanceWithId(teacherTool, criteriaInstanceId);
const catalogCriteriaId = instance?.catalogCriteriaId;
const allCriteria = [...teacherTool.checklist.criteria];
const criteriaIndex = allCriteria.findIndex(c => c.instanceId === criteriaInstanceId);
allCriteria[criteriaIndex].deleted = false;

const newChecklist = {
...teacherTool.checklist,
criteria: allCriteria,
};

setChecklist(newChecklist);

if (catalogCriteriaId) {
pxt.tickEvent(Ticks.RemoveCriteria, { catalogCriteriaId });
Copy link
Contributor

Choose a reason for hiding this comment

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

Is removeCriteria the correct tick to send in this case?

}
}
6 changes: 5 additions & 1 deletion teachertool/src/transforms/removeCriteriaFromChecklist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@ export function removeCriteriaFromChecklist(criteriaInstanceId: string) {

const instance = getCriteriaInstanceWithId(teacherTool, criteriaInstanceId);
const catalogCriteriaId = instance?.catalogCriteriaId;
const allCriteria = [...teacherTool.checklist.criteria];
const criteriaIndex = allCriteria.findIndex(c => c.instanceId === criteriaInstanceId);
allCriteria[criteriaIndex].deleted = true;


const newChecklist = {
...teacherTool.checklist,
criteria: teacherTool.checklist.criteria.filter(c => c.instanceId !== criteriaInstanceId),
criteria: allCriteria,
Copy link
Contributor

@thsparks thsparks Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@thsparks thsparks Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

};

setChecklist(newChecklist);
Expand Down
4 changes: 3 additions & 1 deletion teachertool/src/transforms/runEvaluateAsync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ export async function runEvaluateAsync(fromUserInteraction: boolean) {
setActiveTab("results");
}

const evalRequests = teacherTool.checklist.criteria.map(criteriaInstance =>
const validCriteria = teacherTool.checklist.criteria.filter(c => !c.deleted);

const evalRequests = validCriteria.map(criteriaInstance =>
runSingleEvaluateAsync(criteriaInstance.instanceId, fromUserInteraction)
);

Expand Down
1 change: 1 addition & 0 deletions teachertool/src/types/criteria.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export interface CriteriaInstance {
instanceId: string;
params: CriteriaParameterValue[] | undefined;
userFeedback?: UserFeedback;
deleted?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

// Represents a parameter definition in a catalog criteria.
Expand Down
3 changes: 2 additions & 1 deletion teachertool/src/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ import { classList } from "react-common/components/util";
import { CatalogCriteria } from "../types/criteria";
import { Strings } from "../constants";

export function makeToast(type: ToastType, text: string, timeoutMs: number = 5000): ToastWithId {
export function makeToast(type: ToastType, text: string, timeoutMs: number = 5000, jsx?: React.ReactNode): ToastWithId {
return {
id: nanoid(),
type,
text,
timeoutMs,
jsx,
};
}

Expand Down