Teacher Tool: Improve Customizable Criteria UI (#9915)

This change puts parameters into a clear error-state when they're empty, updates the results view to box in parameters per the design, and contains a few other minor tweaks throughout.

The boxes around parameter values in the results view seemed very similar to the boxes in the catalog view, so I refactored that into a shareable component. The main difference between the two is whether or not you show the parameter name vs the parameter value, which is indicated by whether or not you pass a criteria instance or just a catalog criteria.

teachertool/src/components/CatalogModal.tsx

@@ -6,32 +6,8 @@ import { hideModal } from "../transforms/hideModal";
 import { addCriteriaToRubric } from "../transforms/addCriteriaToRubric";
 import { CatalogCriteria } from "../types/criteria";
 import { getSelectableCatalogCriteria } from "../state/helpers";
+import { ReadOnlyCriteriaDisplay } from "./ReadonlyCriteriaDisplay";
 import css from "./styling/CatalogModal.module.scss";
-import { splitCriteriaTemplate } from "../utils";
-
-interface CatalogCriteriaDisplayProps {
-    criteria: CatalogCriteria;
-}
-const CatalogCriteriaDisplay: React.FC<CatalogCriteriaDisplayProps> = ({ criteria }) => {
-    const segments = useMemo(() => splitCriteriaTemplate(criteria.template), [criteria.template]);
-
-    return (
-        <div className={css["criteria-display"]}>
-            {criteria.template && (
-                <div className={css["criteria-template"]}>
-                    {segments.map((segment, index) => {
-                        return (
-                            <span key={`${criteria.id}-${index}`} className={css[`${segment.type}-segment`]}>
-                                {segment.content}
-                            </span>
-                        );
-                    })}
-                </div>
-            )}
-            {criteria.description && <div className={css["criteria-description"]}>{criteria.description}</div>}
-        </div>
-    );
-};
 
 interface CatalogModalProps {}
 export const CatalogModal: React.FC<CatalogModalProps> = ({}) => {
@@ -96,7 +72,7 @@ export const CatalogModal: React.FC<CatalogModalProps> = ({}) => {
                             id={`checkbox_${criteria.id}`}
                             key={criteria.id}
                             className={css["catalog-item"]}
-                            label={<CatalogCriteriaDisplay criteria={criteria} />}
+                            label={<ReadOnlyCriteriaDisplay catalogCriteria={criteria} showDescription={true} />}
                             onChange={newValue => handleCriteriaSelectedChange(criteria, newValue)}
                             isChecked={isCriteriaSelected(criteria.id)}
                         />

teachertool/src/components/CriteriaInstanceDisplay.tsx

@@ -1,12 +1,13 @@
 import { getCatalogCriteriaWithId } from "../state/helpers";
 import { CriteriaInstance, CriteriaParameterValue } from "../types/criteria";
-import { DebouncedInput } from "./DebouncedInput";
 import { logDebug } from "../services/loggingService";
 import { setParameterValue } from "../transforms/setParameterValue";
 import { classList } from "react-common/components/util";
 import { splitCriteriaTemplate } from "../utils";
 // eslint-disable-next-line import/no-internal-modules
 import css from "./styling/CriteriaInstanceDisplay.module.scss";
+import { useState } from "react";
+import { Input } from "react-common/components/controls/Input";
 
 interface InlineInputSegmentProps {
     initialValue: string;
@@ -22,32 +23,39 @@ const InlineInputSegment: React.FC<InlineInputSegmentProps> = ({
     shouldExpand,
     numeric,
 }) => {
+    const [isEmpty, setIsEmpty] = useState(!initialValue);
+
     function onChange(newValue: string) {
+        setIsEmpty(!newValue);
         setParameterValue(instance.instanceId, param.name, newValue);
     }
 
+    const tooltip = isEmpty ? lf("{0}: value required", param.name) : param.name;
     return (
-        <DebouncedInput
-            className={classList(
-                css["inline-input"],
-                numeric ? css["number-input"] : css["string-input"],
-                shouldExpand ? css["long"] : undefined,
-            )}
-            initialValue={initialValue}
-            onChange={onChange}
-            preserveValueOnBlur={true}
-            placeholder={numeric ? "0" : param.name}
-            title={param.name}
-            autoComplete={false}
-            type = {numeric ? "number" : "text"}
-        />
+        <div title={tooltip} className={css["inline-input-wrapper"]}>
+            <Input
+                className={classList(
+                    css["inline-input"],
+                    numeric ? css["number-input"] : css["string-input"],
+                    shouldExpand ? css["long"] : undefined,
+                    isEmpty ? css["error"] : undefined
+                )}
+                icon={isEmpty ? "fas fa-exclamation-triangle" : undefined}
+                initialValue={initialValue}
+                onChange={onChange}
+                preserveValueOnBlur={true}
+                placeholder={numeric ? "0" : param.name}
+                title={tooltip}
+                autoComplete={false}
+                type={numeric ? "number" : "text"}
+            />
+        </div>
     );
 };
 
 interface CriteriaInstanceDisplayProps {
     criteriaInstance: CriteriaInstance;
 }
-
 export const CriteriaInstanceDisplay: React.FC<CriteriaInstanceDisplayProps> = ({ criteriaInstance }) => {
     const catalogCriteria = getCatalogCriteriaWithId(criteriaInstance.catalogCriteriaId);
     if (!catalogCriteria) {

teachertool/src/components/CriteriaResultEntry.tsx

@@ -9,6 +9,7 @@ import { setEvalResultNotes } from "../transforms/setEvalResultNotes";
 import { CriteriaEvalResultDropdown } from "./CriteriaEvalResultDropdown";
 import { DebouncedTextarea } from "./DebouncedTextarea";
 import { getCatalogCriteriaWithId, getCriteriaInstanceWithId } from "../state/helpers";
+import { ReadOnlyCriteriaDisplay } from "./ReadonlyCriteriaDisplay";
 
 interface AddNotesButtonProps {
     criteriaId: string;
@@ -68,28 +69,20 @@ interface CriteriaResultEntryProps {
 export const CriteriaResultEntry: React.FC<CriteriaResultEntryProps> = ({ criteriaId }) => {
     const { state: teacherTool } = useContext(AppStateContext);
     const [showInput, setShowInput] = useState(!!teacherTool.evalResults[criteriaId]?.notes);
-    const criteriaDisplayString = useRef<string>(getDisplayStringFromCriteriaInstanceId(criteriaId));
 
-    function getDisplayStringFromCriteriaInstanceId(instanceId: string): string {
-        const instance = getCriteriaInstanceWithId(teacherTool, instanceId);
-        if (!instance) {
-            return "";
-        }
-
-        let displayText = getCatalogCriteriaWithId(instance.catalogCriteriaId)?.template ?? "";
-        for (const param of instance.params ?? []) {
-            displayText = displayText.replace(new RegExp(`\\$\\{${param.name}}`, 'i'), param.value);
-        }
-
-        return displayText;
-    }
+    const criteriaInstance = getCriteriaInstanceWithId(teacherTool, criteriaId);
+    const catalogCriteria = criteriaInstance ? getCatalogCriteriaWithId(criteriaInstance.catalogCriteriaId) : undefined;
 
     return (
         <>
-            {criteriaDisplayString.current && (
+            {catalogCriteria && (
                 <div className={css["specific-criteria-result"]} key={criteriaId}>
                     <div className={css["result-details"]}>
-                        <h4 className={css["display-string"]}>{criteriaDisplayString.current}</h4>
+                        <ReadOnlyCriteriaDisplay
+                            catalogCriteria={catalogCriteria}
+                            criteriaInstance={criteriaInstance}
+                            showDescription={false}
+                        />
                         <CriteriaEvalResultDropdown
                             result={teacherTool.evalResults[criteriaId].result}
                             criteriaId={criteriaId}

teachertool/src/components/ReadonlyCriteriaDisplay.tsx

@@ -0,0 +1,49 @@
+import { useContext, useMemo } from "react";
+import { CatalogCriteria, CriteriaInstance } from "../types/criteria";
+import { splitCriteriaTemplate } from "../utils";
+import { CriteriaTemplateSegment } from "../types";
+import { getParameterValue } from "../state/helpers";
+import { AppStateContext } from "../state/appStateContext";
+import css from "./styling/ReadonlyCriteriaDisplay.module.scss";
+
+export interface ReadOnlyCriteriaDisplayProps {
+    catalogCriteria: CatalogCriteria;
+    criteriaInstance?: CriteriaInstance; // If defined, show parameter values in the display. Else, show parameter names.
+    showDescription?: boolean;
+}
+
+export const ReadOnlyCriteriaDisplay: React.FC<ReadOnlyCriteriaDisplayProps> = ({
+    catalogCriteria,
+    criteriaInstance,
+    showDescription,
+}) => {
+    const { state: teacherTool } = useContext(AppStateContext);
+
+    function getSegmentDisplayText(segment: CriteriaTemplateSegment): string {
+        if (!criteriaInstance || segment.type === "plain-text") {
+            return segment.content;
+        }
+
+        return getParameterValue(teacherTool, criteriaInstance.instanceId, segment.content) ?? "";
+    }
+
+    const segments = useMemo(() => splitCriteriaTemplate(catalogCriteria.template), [catalogCriteria.template]);
+    return (
+        <div className={css["criteria-display"]}>
+            {catalogCriteria.template && (
+                <div className={css["criteria-template"]}>
+                    {segments.map((segment, index) => {
+                        return (
+                            <span key={index} className={css[`${segment.type}-segment`]}>
+                                {getSegmentDisplayText(segment)}
+                            </span>
+                        );
+                    })}
+                </div>
+            )}
+            {showDescription && catalogCriteria.description && (
+                <div className={css["criteria-description"]}>{catalogCriteria.description}</div>
+            )}
+        </div>
+    );
+};

teachertool/src/components/styling/CatalogModal.module.scss

@@ -11,27 +11,4 @@
     .catalog-item {
         padding: 0.5rem;
     }
-
-    .criteria-display {
-        display: flex;
-        flex-direction: column;
-        align-items: flex-start;
-
-        .criteria-template {
-            .plain-text-segment {
-                margin-right: 0.3rem;
-            }
-
-            .param-segment {
-                border-radius: 0.5rem;
-                border: 1px solid var(--pxt-page-foreground);
-                padding: 0.2rem 0.5rem;
-                margin-right: 0.3rem;
-            }
-        }
-
-        .criteria-description {
-            color: var(--pxt-page-foreground-light);
-        }
-    }
 }

teachertool/src/components/styling/CriteriaInstanceDisplay.module.scss

@@ -8,7 +8,7 @@
         border-radius: 0.5rem;
         border: solid 1px var(--pxt-page-foreground);
         padding: 0 0.4rem;
-        
+
         &:focus::after {
             outline: none;
             border-radius: 0.5rem;
@@ -42,38 +42,100 @@
                 flex-grow: 1;
             }
 
-            .inline-input {
-                margin: 0.2rem 0;
+            .inline-input-wrapper {
+                &:has(.long) {
+                    flex-grow: 1;
+                }
+
+                .inline-input {
+                    margin: 0.2rem 0;
+
+                    &.string-input {
+                        width: 10rem;
 
-                &.string-input {
-                    width: 10rem;
-
-                    &.long {
-                        flex-grow: 1;
-                        input {
-                            text-align: start;
+                        &.long {
+                            width: unset;
+                            flex-grow: 1;
+
+                            input {
+                                text-align: start;
+                            }
+                        }
+
+                        &.error:not(:focus):not(:focus-within) input {
+                            margin-left: 1.8rem;
                         }
                     }
-                }
-
-                &.number-input {
-                    width: 2.3rem;
-
-                    // Hide arrows on right-hand side of number input.
-                    // Firefox
-                    input[type=number] {
-                        -moz-appearance: textfield;
+
+                    &.number-input {
+                        width: 2.7rem; // Just enough space for 3 digits
+
+                        &.error:not(:focus):not(:focus-within) {
+                            width: 4rem; // Add space for icon
+
+                            input {
+                                margin-left: 1.3rem;
+                            }
+
+                            i {
+                                bottom: 0.4rem;
+                            }
+                        }
+
+                        // Hide arrows on right-hand side of number input.
+                        // Firefox
+                        input[type=number] {
+                            -moz-appearance: textfield;
+                        }
+
+                        // Other browsers
+                        input::-webkit-outer-spin-button,
+                        input::-webkit-inner-spin-button {
+                            -webkit-appearance: none;
+                            margin: 0;
+                        }
                     }
-                    // Other browsers
-                    input::-webkit-outer-spin-button, input::-webkit-inner-spin-button {
-                        -webkit-appearance: none;
-                        margin: 0;
+
+                    input {
+                        text-align: center;
+                        padding: 0;
                     }
-                }
 
-                input {
-                    text-align: center;
-                    padding: 0;
+                    input {
+                        transition: margin-left 0.05s ease-in-out;
+                    }
+
+                    &.error {
+                        &:not(:focus):not(:focus-within) div[class*="common-input-group"] {
+                            border: solid 1px var(--pxt-error-accent);
+                            background-color: var(--pxt-error-background);
+                            transition: background-color 0.3s ease-in-out, border 0.3s ease-in-out;
+
+                            &:focus::after {
+                                border: solid 2px var(--pxt-error);
+                            }
+
+                            &:focus-within::after {
+                                border: solid 2px var(--pxt-error);
+                            }
+
+                            input::placeholder {
+                                color: var(--pxt-page-foreground);
+                            }
+
+                            i {
+                                right: unset;
+                                left: 0.5rem;
+                            }
+                        }
+
+                        &:focus,
+                        &:focus-within {
+                            i {
+                                display: none;
+                            }
+                        }
+                    }
                 }
             }
         }

teachertool/src/components/styling/ReadonlyCriteriaDisplay.module.scss

@@ -0,0 +1,22 @@
+.criteria-display {
+    display: flex;
+    flex-direction: column;
+    align-items: flex-start;
+
+    .criteria-template {
+        .plain-text-segment {
+            margin-right: 0.3rem;
+        }
+
+        .param-segment {
+            border-radius: 0.5rem;
+            border: 1px solid var(--pxt-page-foreground);
+            padding: 0.2rem 0.5rem;
+            margin-right: 0.3rem;
+        }
+    }
+
+    .criteria-description {
+        color: var(--pxt-page-foreground-light);
+    }
+}