Add Tags to Catalog Criteria #10007

thsparks · 2024-05-07T21:55:01Z

This change adds tags to criteria in the catalog. The schema is setup to support criteria having multiple tags, but for now we only adjust our display based on the first tag. This may change if we add more criteria and start using more granular tags, but it's not necessary given the current catalog.

If a criterion doesn't have any tags, it gets assigned an "Other" tag. Currently, all visible catalog criteria have tags, so the other tag is never shown.

Initially, the first tag in the catalog is expanded, but the tool remembers which tags the user expands/collapses and preserves those for the next time the catalog is opened.

I made a few adjustments to the accordion display to support this change:

A flag to allow multiple items to be expanded
The ability to specify an initial "open" item set
The optional ability to specify ids for each item (to support number 2)

Upload target is here, but since it's looking at production docs, everything is just placed into the "Other" category for now: https://makecode.microbit.org/app/13ea77ed2e1da12176c4c12a9907981375709c43-71c65280c6--eval

Screenshots (from localhost with updated docs)

Fixes microsoft/pxt-microbit#5597

srietkerk · 2024-05-08T22:54:54Z

react-common/components/controls/Accordion/Accordion.tsx

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


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

srietkerk · 2024-05-08T22:56:14Z

react-common/components/controls/Accordion/context.tsx

 type ClearExpanded = {
    type: "CLEAR_EXPANDED";
 };

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


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

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

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.

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

srietkerk · 2024-05-08T23:08:51Z

teachertool/src/services/storageService.ts

+export async function removeExpandedCatalogTagAsync(tag: string) {
+    let expandedTags = getExpandedCatalogTags();
+    if (!expandedTags) {
+        await setExpandedCatalogTags([]);


Why is it needed that we set the expanded tags to an empty list if getExpandedCatalogTags() returns undefined?

If someone is trying to remove a saved expanded tag but there were none saved to begin with, it's a pretty bizarre scenario to begin with, but I decided to switch it over to an empty list (to save state as "all categories collapsed") rather than undefined, which is mostly just the initial "nothing has happened yet" state. It's pretty arbitrary handling of a shouldn't-ever-happen edge case.

teachertool/src/components/CatalogOverlay.tsx

srietkerk · 2024-05-08T23:17:04Z

teachertool/src/components/CatalogOverlay.tsx

+
+    const tags = Object.keys(criteriaGroupedByTag);
+    if (tags.length === 0) {
+        return null;


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

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.

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?

Added error log

srietkerk · 2024-05-08T23:19:50Z

teachertool/src/services/storageService.ts

+    }
+}
+
+export async function addExandedCatalogTagAsync(tag: string) {


Suggested change

export async function addExandedCatalogTagAsync(tag: string) {

export async function addExpandedCatalogTagAsync(tag: string) {

srietkerk · 2024-05-08T23:20:11Z

teachertool/src/components/CatalogOverlay.tsx

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


Suggested change

addExandedCatalogTagAsync(tags[0]);

addExpandedCatalogTagAsync(tags[0]);

srietkerk · 2024-05-08T23:25:08Z

teachertool/src/components/CatalogOverlay.tsx

+                        <Accordion.Header>{tag}</Accordion.Header>
+                        <Accordion.Panel>
+                            {criteriaGroupedByTag[tag].map(c => {
+                                const existingInstanceCount = teacherTool.checklist.criteria.filter(


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.

srietkerk · 2024-05-08T23:28:27Z

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

@@ -77,7 +90,7 @@
                    .action-indicators {
                        position: relative;
                        padding: 0 1rem 0 0;
-                        width: 3rem;
+                        width: 3rem; // Match common-accordion-chevron


Is this comment needed?

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.

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.

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.

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

riknoll · 2024-05-08T23:21:18Z

react-common/components/controls/Accordion/context.tsx

+    type: "REMOVE_EXPANDED";
+    id: string;
+};
+
 type ClearExpanded = {


is this used anymore? can probably be removed

riknoll · 2024-05-08T23:27:22Z

teachertool/src/components/CatalogOverlay.tsx

+        return `accordion-item-${tag}`;
+    }
+
+    function onTagExpandToggled(tag: string, expanded: boolean) {


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

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

riknoll · 2024-05-08T23:29:12Z

teachertool/src/components/CatalogOverlay.tsx

+                                return (
+                                    c.template && (
+                                        <Button
+                                            id={`criteria_${c.id}`}


is this id used anywhere?

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

teachertool/src/components/CatalogOverlay.tsx

srietkerk

Looks great!

thsparks added 14 commits May 7, 2024 10:31

Basic Accordion Implementation for Catalog, no styling

ea2a2af

Initial styling

1dd5a1a

Support multiple expanded panels in the accordion

13d306d

Default open initial category

add5f06

Remove unused imports

afc4850

Reorder imports

5d8408a

Formatting

8dbd82c

Save expanded categories

0ae19f3

Persist all tags collapsed

a2183f5

Formatting and tidy up imports

4044655

Prettier

f981ff3

Use lf and constant for default category

280b416

Fix horizontal wrapping

4a6be94

Adjust error handling in case tags list is empty

3da27ac

thsparks requested a review from a team May 7, 2024 21:55

thsparks mentioned this pull request May 7, 2024

Add tags to visible criteria microsoft/pxt-microbit#5656

Merged

Make text align in headers and in criteria

8ba07dc

srietkerk reviewed May 8, 2024

View reviewed changes

riknoll reviewed May 8, 2024

View reviewed changes

thsparks added 3 commits May 8, 2024 16:32

!== instead of !=

ee69a7d

Remove clear expanded

ad42cbf

Fix typo in function name

28fdf3a

srietkerk reviewed May 8, 2024

View reviewed changes

teachertool/src/components/CatalogOverlay.tsx Show resolved Hide resolved

thsparks added 5 commits May 8, 2024 17:05

Create CatalogItem to encapsulate single entry in accordion panel

c10516b

Remove unused id

5f15a26

Add async to function name

a58b3a3

Remove unnecessary async stuff

c169068

Add error log

213491a

srietkerk approved these changes May 9, 2024

View reviewed changes

thsparks merged commit 1b14c18 into master May 10, 2024
6 checks passed

thsparks deleted the thsparks/teachertool/tags branch May 10, 2024 01:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tags to Catalog Criteria #10007

Add Tags to Catalog Criteria #10007

thsparks commented May 7, 2024 •

edited

srietkerk May 8, 2024

srietkerk May 8, 2024

thsparks May 8, 2024 •

edited

srietkerk May 8, 2024

thsparks May 9, 2024 •

edited

srietkerk May 8, 2024

thsparks May 8, 2024

srietkerk May 8, 2024

thsparks May 8, 2024

srietkerk May 8, 2024

thsparks May 9, 2024

srietkerk May 8, 2024

srietkerk May 8, 2024

srietkerk May 8, 2024

srietkerk May 8, 2024

thsparks May 8, 2024

srietkerk May 9, 2024

thsparks May 9, 2024

srietkerk May 9, 2024

riknoll May 8, 2024

riknoll May 8, 2024

thsparks May 9, 2024

riknoll May 8, 2024

thsparks May 9, 2024

srietkerk left a comment

	export async function addExandedCatalogTagAsync(tag: string) {
	export async function addExpandedCatalogTagAsync(tag: string) {

	addExandedCatalogTagAsync(tags[0]);
	addExpandedCatalogTagAsync(tags[0]);

Add Tags to Catalog Criteria #10007

Add Tags to Catalog Criteria #10007

Conversation

thsparks commented May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thsparks May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thsparks May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srietkerk left a comment

Choose a reason for hiding this comment

thsparks commented May 7, 2024 •

edited

thsparks May 8, 2024 •

edited

thsparks May 9, 2024 •

edited