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 all 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
31 changes: 29 additions & 2 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,9 @@ 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";
import { softDeleteCriteriaFromChecklist } from "../transforms/softDeleteCriteriaFromChecklist";

interface CriteriaResultNotesProps {
criteriaId: string;
Expand Down Expand Up @@ -76,6 +79,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 +113,14 @@ const CriteriaResultToolbarTray: React.FC<{ criteriaId: string }> = ({ criteriaI
}

async function handleDeleteClickedAsync() {
if (confirm(Strings.ConfirmDeleteCriteriaInstance)) {
const toastTimeout = 5000;
softDeleteCriteriaFromChecklist(criteriaId);
const toast = makeToast("info", Strings.CriteriaDeleted, toastTimeout);
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);
setTimeout(() => {
removeCriteriaFromChecklist(criteriaId);
}
}, toastTimeout + 500);
}

return (
Expand Down
9 changes: 4 additions & 5 deletions teachertool/src/components/EvalResultDisplay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@ import { setChecklistName } from "../transforms/setChecklistName";
import { Strings, Ticks } from "../constants";
import { Button } from "react-common/components/controls/Button";
import { runEvaluateAsync } from "../transforms/runEvaluateAsync";
import { isProjectLoaded } from "../state/helpers";
import { showToast } from "../state/actions";
import { makeToast } from "../utils";
import { getSafeChecklistName } from "../state/helpers";
import { isProjectLoaded, getSafeChecklistName, getActiveCriteria } from "../state/helpers";
import { writeChecklistToFile } from "../services/fileSystemService";

interface ResultsHeaderProps {
Expand All @@ -27,6 +26,7 @@ const ResultsHeader: React.FC<ResultsHeaderProps> = ({ printRef }) => {
const { state: teacherTool } = useContext(AppStateContext);
const { checklist, projectMetadata } = teacherTool;
let { name: checklistName } = checklist;
const criteriaToExport = getActiveCriteria(teacherTool);
const [checklistNameInputRef, setChecklistNameInputRef] = useState<HTMLInputElement>();

const handleEvaluateClickedAsync = async () => {
Expand All @@ -49,7 +49,7 @@ const ResultsHeader: React.FC<ResultsHeaderProps> = ({ printRef }) => {

const handleExportChecklistClicked = () => {
pxt.tickEvent(Ticks.ExportChecklist);
writeChecklistToFile(checklist);
writeChecklistToFile({ name: checklistName, criteria: criteriaToExport });
};

return (
Expand Down Expand Up @@ -115,8 +115,7 @@ const ResultsHeader: React.FC<ResultsHeaderProps> = ({ printRef }) => {

const CriteriaWithResultsTable: React.FC = () => {
const { state: teacherTool } = useContext(AppStateContext);
const { checklist } = teacherTool;
const { criteria } = checklist;
const criteria = getActiveCriteria(teacherTool);

return (
<div className={css["results-list"]}>
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
4 changes: 4 additions & 0 deletions teachertool/src/state/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ export function getCatalogCriteriaWithId(id: string): CatalogCriteria | undefine
return state.catalog?.find(c => c.id === id);
}

export function getActiveCriteria(state: AppState): CriteriaInstance[] {
return state.checklist.criteria.filter(c => !c.deleted);
}

export function getCriteriaInstanceWithId(state: AppState, id: string): CriteriaInstance | undefined {
return state.checklist.criteria.find(c => c.instanceId === id);
}
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) {
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.

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.

}
}
5 changes: 4 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 @@ -14,7 +14,10 @@ export function removeCriteriaFromChecklist(criteriaInstanceId: string) {

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

Choose a reason for hiding this comment

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

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

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 the c.instanceId === criteriaInstanceId check is redundant. Could just be c.instanceId !== criteriaInstanceId || !c.deleted

),
};

setChecklist(newChecklist);
Expand Down
5 changes: 4 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 @@ -4,6 +4,7 @@ import { showToast } from "./showToast";
import { setActiveTab } from "./setActiveTab";
import { runSingleEvaluateAsync } from "./runSingleEvaluateAsync";
import { Strings } from "../constants";
import { getActiveCriteria } from "../state/helpers";

export async function runEvaluateAsync(fromUserInteraction: boolean) {
const { state: teacherTool } = stateAndDispatch();
Expand All @@ -17,7 +18,9 @@ export async function runEvaluateAsync(fromUserInteraction: boolean) {
setActiveTab("results");
}

const evalRequests = teacherTool.checklist.criteria.map(criteriaInstance =>
const activeCriteria = getActiveCriteria(teacherTool);

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

Expand Down
29 changes: 29 additions & 0 deletions teachertool/src/transforms/softDeleteCriteriaFromChecklist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { stateAndDispatch } from "../state";
import { logDebug } from "../services/loggingService";
import { setChecklist } from "./setChecklist";
import { Ticks } from "../constants";
import { getCriteriaInstanceWithId } from "../state/helpers";

export function softDeleteCriteriaFromChecklist(criteriaInstanceId: string) {
const { state: teacherTool } = stateAndDispatch();

logDebug(`Soft deleting 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 = true;


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

setChecklist(newChecklist);

if (catalogCriteriaId) {
pxt.tickEvent(Ticks.RemoveCriteria, { catalogCriteriaId });
}
}
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