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 15 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be !== instead of !=?


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>
);
}
}
44 changes: 33 additions & 11 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,16 @@ type SetExpanded = {
id: string;
};

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

type ClearExpanded = {
Copy link
Member

Choose a reason for hiding this comment

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

is this used anymore? can probably be removed

type: "CLEAR_EXPANDED";
};

type Action = SetExpanded | ClearExpanded;
type Action = SetExpanded | RemoveExpanded | ClearExpanded;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we keeping ClearExpanded for a feature that allows a user to collapse all expanded categories?

Copy link
Contributor Author

@thsparks thsparks May 8, 2024

Choose a reason for hiding this comment

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

Yeah, I just thought it might be useful later on. I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually on board with keeping this in for the component's sake. I think it's a very common use case that a user wants to collapse all the expanded categories when there's too much going on, I just wanted to check in that that was the reason why it was left in.

Copy link
Contributor Author

@thsparks thsparks May 9, 2024

Choose a reason for hiding this comment

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

I already removed it 😉 but should be easy to re-add if someone needs it down the road


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

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

export const clearExpanded = (): ClearExpanded => (
{
type: "CLEAR_EXPANDED"
}
);

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

export function useAccordionDispatch() {
Expand All @@ -59,12 +76,17 @@ 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 "REMOVE_EXPANDED":
return {
...state,
expanded: state.expanded.filter((id) => id !== action.id),
};
case "CLEAR_EXPANDED":
return {
...state,
expanded: undefined
expanded: undefined,
};
}
}
}
117 changes: 88 additions & 29 deletions teachertool/src/components/CatalogOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,19 @@ 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 {
addExandedCatalogTagAsync,
getExpandedCatalogTags,
removeExpandedCatalogTagAsync,
} from "../services/storageService";
import css from "./styling/CatalogOverlay.module.scss";

interface CatalogHeaderProps {
Expand Down Expand Up @@ -73,10 +81,23 @@ const CatalogList: React.FC = () => {
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 +127,72 @@ 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) {
/* await */ addExandedCatalogTagAsync(tag);
srietkerk marked this conversation as resolved.
Show resolved Hide resolved
} else {
/* await */ removeExpandedCatalogTagAsync(tag);
}
}

const tags = Object.keys(criteriaGroupedByTag);
if (tags.length === 0) {
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.
addExandedCatalogTagAsync(tags[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
addExandedCatalogTagAsync(tags[0]);
addExpandedCatalogTagAsync(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
catalogCriteria={c}
isMaxed={isMaxed}
recentlyAdded={recentlyAddedIds[c.id] !== undefined}
/>
}
onClick={() => onItemClicked(c)}
disabled={isMaxed}
/>
)
<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 => {
const existingInstanceCount = teacherTool.checklist.criteria.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should break out these catalog entries into its own component like CatalogOverlayEntry or something like that to keep the accordion area clean.

i => i.catalogCriteriaId === c.id
).length;
const isMaxed = c.maxCount !== undefined && existingInstanceCount >= c.maxCount;
return (
c.template && (
<Button
id={`criteria_${c.id}`}
Copy link
Member

Choose a reason for hiding this comment

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

is this id used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think it is, actually. I'll remove it.

title={getReadableCriteriaTemplate(c)}
key={c.id}
className={css["catalog-item"]}
label={
<CatalogItemLabel
catalogCriteria={c}
isMaxed={isMaxed}
recentlyAdded={recentlyAddedIds[c.id] !== undefined}
/>
}
onClick={() => onItemClicked(c)}
disabled={isMaxed}
/>
)
);
})}
</Accordion.Panel>
</Accordion.Item>
);
})}
</div>
</Accordion>
);
};

Expand Down
19 changes: 16 additions & 3 deletions teachertool/src/components/styling/CatalogOverlay.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
align-items: center;

.catalog-content-container {
max-width: 95%;
max-height: 95%;
width: 95%;
height: 95%;
background-color: var(--pxt-page-background);
border-radius: .285rem; // Match modal
display: flex;
Expand Down Expand Up @@ -57,6 +57,19 @@
display: flex;
flex-direction: column;
width: 100%;
height: 100%;

div[class*="common-accordion-chevron"] {
width: 3rem; // Match action-indicators
}

button[class*="common-accordion-header-outer"] {
background-color: var(--pxt-content-foreground);
color: var(--pxt-content-background);
border-bottom: 1px solid var(--pxt-content-accent);
font-size: 1.2rem;
padding: 0.5rem 0.95rem;
}

.catalog-item {
width: 100%;
Expand All @@ -77,7 +90,7 @@
.action-indicators {
position: relative;
padding: 0 1rem 0 0;
width: 3rem;
width: 3rem; // Match common-accordion-chevron
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment needed?

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 think it's a nice reminder if someone ever changes it, they should consider changing it there too (which may or may not look good depending on the new value, up to the engineer at that point). Could go and do a whole var for it but that seemed like overkill to me.

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 not sure if that warrants a comment, personally. I feel like there is a lot of styling that is made in order to match with styling elsewhere, and leaving that up to experimentation is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I were the engineer making the change, I would appreciate the comment because it's the kind of thing I'd otherwise miss :) But if you feel very strongly I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, don't feel strongly; fine to keep it in.

display: flex;
align-items: center;
justify-content: center;
Expand Down
Loading