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.

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

Add Tags to Catalog Criteria #10007

Merged
merged 23 commits into from
May 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions common-docs/teachertool/catalog-shared.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"template": "${Block} used ${count} times",
"description": "This block was used the specified number of times in your project.",
"docPath": "/teachertool",
"tags": ["General"],
"params": [
{
"name": "block",
Expand All @@ -27,6 +28,7 @@
"description": "The project contains at least the specified number of comments.",
"docPath": "/teachertool",
"maxCount": 1,
"tags": ["General"],
"params": [
{
"name": "count",
Expand All @@ -43,6 +45,7 @@
"docPath": "/teachertool",
"description": "The program uses at least this many loops of any kind (for, repeat, while, or for-of).",
"maxCount": 1,
"tags": ["Code Elements"],
"params": [
{
"name": "count",
Expand All @@ -59,6 +62,7 @@
"docPath": "/teachertool",
"description": "At least this many user-defined functions are created and called.",
"maxCount": 1,
"tags": ["Code Elements"],
"params": [
{
"name": "count",
Expand All @@ -75,6 +79,7 @@
"docPath": "/teachertool",
"description": "The program creates and uses at least this many user-defined variables.",
"maxCount": 1,
"tags": ["Code Elements"],
"params": [
{
"name": "count",
Expand Down
1 change: 1 addition & 0 deletions common-docs/teachertool/test/catalog-shared.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"description": "Experimental: AI outputs may not be accurate. Use with caution and always review responses.",
"docPath": "/teachertool",
"maxCount": 10,
"tags": ["General"],
"params": [
{
"name": "question",
Expand Down
23 changes: 16 additions & 7 deletions react-common/components/controls/Accordion/Accordion.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import * as React from "react";
import { ContainerProps, classList, fireClickOnEnter } from "../../util";
import { useId } from "../../../hooks/useId";
import { AccordionProvider, clearExpanded, setExpanded, useAccordionDispatch, useAccordionState } from "./context";
import { AccordionProvider, removeExpanded, setExpanded, useAccordionDispatch, useAccordionState } from "./context";

export interface AccordionProps extends ContainerProps {
multiExpand?: boolean;
defaultExpandedIds?: string[];
children?: React.ReactElement<AccordionItemProps>[] | React.ReactElement<AccordionItemProps>;
}

export interface AccordionItemProps extends ContainerProps {
children?: [React.ReactElement<AccordionHeaderProps>, React.ReactElement<AccordionPanelProps>];
noChevron?: boolean;
itemId?: string;
onExpandToggled?: (expanded: boolean) => void;
}

export interface AccordionHeaderProps extends ContainerProps {
Expand All @@ -27,10 +31,12 @@ export const Accordion = (props: AccordionProps) => {
ariaHidden,
ariaDescribedBy,
role,
multiExpand,
defaultExpandedIds
} = props;

return (
<AccordionProvider>
<AccordionProvider multiExpand={multiExpand} defaultExpandedIds={defaultExpandedIds}>
<div
className={classList("common-accordion", className)}
id={id}
Expand All @@ -54,23 +60,26 @@ export const AccordionItem = (props: AccordionItemProps) => {
ariaHidden,
ariaDescribedBy,
role,
noChevron
noChevron,
itemId,
onExpandToggled,
} = props;

const { expanded } = useAccordionState();
const dispatch = useAccordionDispatch();

const panelId = useId();
const panelId = itemId ?? useId();
const mappedChildren = React.Children.toArray(children);
const isExpanded = expanded === panelId;
const isExpanded = expanded.indexOf(panelId) !== -1;

const onHeaderClick = React.useCallback(() => {
if (isExpanded) {
dispatch(clearExpanded());
dispatch(removeExpanded(panelId));
}
else {
dispatch(setExpanded(panelId));
}
onExpandToggled?.(!isExpanded);
}, [isExpanded]);

return (
Expand Down Expand Up @@ -150,4 +159,4 @@ export const AccordionPanel = (props: AccordionPanelProps) => {
{children}
</div>
);
}
}
39 changes: 23 additions & 16 deletions react-common/components/controls/Accordion/context.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
import * as React from "react";

interface AccordionState {
expanded?: string;
multiExpand?: boolean;
expanded: string[];
}

const AccordionStateContext = React.createContext<AccordionState>(null);
const AccordionDispatchContext = React.createContext<(action: Action) => void>(null);

export const AccordionProvider = ({ children }: React.PropsWithChildren<{}>) => {
const [state, dispatch] = React.useReducer(
accordionReducer,
{}
);
export const AccordionProvider = ({
multiExpand,
defaultExpandedIds,
children,
}: React.PropsWithChildren<{ multiExpand?: boolean; defaultExpandedIds?: string[] }>) => {
const [state, dispatch] = React.useReducer(accordionReducer, {
expanded: defaultExpandedIds ?? [],
multiExpand,
});

return (
<AccordionStateContext.Provider value={state}>
Expand All @@ -27,11 +32,12 @@ type SetExpanded = {
id: string;
};

type ClearExpanded = {
type: "CLEAR_EXPANDED";
type RemoveExpanded = {
type: "REMOVE_EXPANDED";
id: string;
};

type Action = SetExpanded | ClearExpanded;
type Action = SetExpanded | RemoveExpanded;

export const setExpanded = (id: string): SetExpanded => (
{
Expand All @@ -40,14 +46,15 @@ export const setExpanded = (id: string): SetExpanded => (
}
);

export const clearExpanded = (): ClearExpanded => (
export const removeExpanded = (id: string): RemoveExpanded => (
{
type: "CLEAR_EXPANDED"
type: "REMOVE_EXPANDED",
id
}
);

export function useAccordionState() {
return React.useContext(AccordionStateContext)
return React.useContext(AccordionStateContext);
}

export function useAccordionDispatch() {
Expand All @@ -59,12 +66,12 @@ function accordionReducer(state: AccordionState, action: Action): AccordionState
case "SET_EXPANDED":
return {
...state,
expanded: action.id
expanded: state.multiExpand ? [...state.expanded, action.id] : [action.id],
};
case "CLEAR_EXPANDED":
case "REMOVE_EXPANDED":
return {
...state,
expanded: undefined
expanded: state.expanded.filter((id) => id !== action.id),
};
}
}
}
122 changes: 95 additions & 27 deletions teachertool/src/components/CatalogOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@ import { getCatalogCriteria } from "../state/helpers";
import { ReadOnlyCriteriaDisplay } from "./ReadonlyCriteriaDisplay";
import { Strings } from "../constants";
import { Button } from "react-common/components/controls/Button";
import { getReadableCriteriaTemplate, makeToast } from "../utils";
import { Accordion } from "react-common/components/controls/Accordion";
import { getReadableCriteriaTemplate } from "../utils";
import { setCatalogOpen } from "../transforms/setCatalogOpen";
import { classList } from "react-common/components/util";
import { announceToScreenReader } from "../transforms/announceToScreenReader";
import { FocusTrap } from "react-common/components/controls/FocusTrap";
import { logError } from "../services/loggingService";
import { ErrorCode } from "../types/errorCode";
import { addExpandedCatalogTag, getExpandedCatalogTags, removeExpandedCatalogTag } from "../services/storageService";
import css from "./styling/CatalogOverlay.module.scss";

interface CatalogHeaderProps {
Expand Down Expand Up @@ -67,16 +71,59 @@ const CatalogItemLabel: React.FC<CatalogItemLabelProps> = ({ catalogCriteria, is
);
};

interface CatalogItemProps {
catalogCriteria: CatalogCriteria;
recentlyAddedIds: pxsim.Map<NodeJS.Timeout>;
onItemClicked: (c: CatalogCriteria) => void;
}
const CatalogItem: React.FC<CatalogItemProps> = ({ catalogCriteria, recentlyAddedIds, onItemClicked }) => {
const { state: teacherTool } = useContext(AppStateContext);

const existingInstanceCount = teacherTool.checklist.criteria.filter(
i => i.catalogCriteriaId === catalogCriteria.id
).length;
const isMaxed = catalogCriteria.maxCount !== undefined && existingInstanceCount >= catalogCriteria.maxCount;
return catalogCriteria.template ? (
<Button
title={getReadableCriteriaTemplate(catalogCriteria)}
key={catalogCriteria.id}
className={css["catalog-item"]}
label={
<CatalogItemLabel
catalogCriteria={catalogCriteria}
isMaxed={isMaxed}
recentlyAdded={recentlyAddedIds[catalogCriteria.id] !== undefined}
/>
}
onClick={() => onItemClicked(catalogCriteria)}
disabled={isMaxed}
/>
) : null;
};

const CatalogList: React.FC = () => {
const { state: teacherTool } = useContext(AppStateContext);

const recentlyAddedWindowMs = 500;
const [recentlyAddedIds, setRecentlyAddedIds] = useState<pxsim.Map<NodeJS.Timeout>>({});

const criteria = useMemo<CatalogCriteria[]>(
() => getCatalogCriteria(teacherTool),
[teacherTool.catalog, teacherTool.checklist]
);
// For now, we only look at the first tag of each criteria.
const criteriaGroupedByTag = useMemo<pxt.Map<CatalogCriteria[]>>(() => {
const grouped: pxt.Map<CatalogCriteria[]> = {};
getCatalogCriteria(teacherTool)?.forEach(c => {
if (!c.tags || c.tags.length === 0) {
logError(ErrorCode.missingTag, { message: "Catalog criteria missing tag", criteria: c });
return;
}

const tag = c.tags[0];
if (!grouped[tag]) {
grouped[tag] = [];
}
grouped[tag].push(c);
});
return grouped;
}, [teacherTool.catalog]);

function updateRecentlyAddedValue(id: string, value: NodeJS.Timeout | undefined) {
setRecentlyAddedIds(prevState => {
Expand Down Expand Up @@ -106,34 +153,55 @@ const CatalogList: React.FC = () => {
announceToScreenReader(lf("Added '{0}' to checklist.", getReadableCriteriaTemplate(c)));
}

function getItemIdForTag(tag: string) {
return `accordion-item-${tag}`;
}

function onTagExpandToggled(tag: string, expanded: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

this being async seems dangerous. looking at the implementation below of these two functions, it seems like hammering on this button could cause some issues with multiple copies of the same tag being added to the state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true...I'll just remove the async stuff. Most of our local storage stuff isn't async anyway.

if (expanded) {
addExpandedCatalogTag(tag);
} else {
removeExpandedCatalogTag(tag);
}
}

const tags = Object.keys(criteriaGroupedByTag);
if (tags.length === 0) {
logError(ErrorCode.noCatalogCriteria);
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, does this mean that the overlay will be empty if there were no tags defined?

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. It'd mean there were no criteria, since every criteria (once loaded into state) should have at least one tag. Should never happen, but better this than some random error, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, makes sense. Maybe we should log something, too? Makes sense that it should never happen, but I feel like if there was a blank overlay and someone looked at the console and no problems were reported, it would feel very buggy. Although I'm guessing there should be other errors in the console if that were to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added error log

}

let expandedTags = getExpandedCatalogTags();
if (!expandedTags) {
// If we haven't saved an expanded set, default expand the first one.
addExpandedCatalogTag(tags[0]);
expandedTags = [tags[0]];
}

const expandedIds = expandedTags.map(t => getItemIdForTag(t));
return (
<div className={css["catalog-list"]}>
{criteria.map(c => {
const existingInstanceCount = teacherTool.checklist.criteria.filter(
i => i.catalogCriteriaId === c.id
).length;
const isMaxed = c.maxCount !== undefined && existingInstanceCount >= c.maxCount;
<Accordion className={css["catalog-list"]} multiExpand={true} defaultExpandedIds={expandedIds}>
{tags.map(tag => {
return (
c.template && (
<Button
id={`criteria_${c.id}`}
title={getReadableCriteriaTemplate(c)}
key={c.id}
className={css["catalog-item"]}
label={
<CatalogItemLabel
<Accordion.Item
itemId={getItemIdForTag(tag)}
onExpandToggled={expanded => onTagExpandToggled(tag, expanded)}
key={getItemIdForTag(tag)}
>
<Accordion.Header>{tag}</Accordion.Header>
srietkerk marked this conversation as resolved.
Show resolved Hide resolved
<Accordion.Panel>
{criteriaGroupedByTag[tag].map(c => (
<CatalogItem
catalogCriteria={c}
isMaxed={isMaxed}
recentlyAdded={recentlyAddedIds[c.id] !== undefined}
recentlyAddedIds={recentlyAddedIds}
onItemClicked={onItemClicked}
/>
}
onClick={() => onItemClicked(c)}
disabled={isMaxed}
/>
)
))}
</Accordion.Panel>
</Accordion.Item>
);
})}
</div>
</Accordion>
);
};

Expand Down
Loading