Teacher Tool: Make Catalog Non-Modal #9983
Conversation
…ks/teachertool/rework_catalog_modal
…sing components to get redefined whenever re-renders happened, which caused flickering and issues with persistence (like scrolling getting reset)
|
Feels nice! I like the little Plus icon to add a rule. I also like that I can add a blocks exist rule and then select which block I want without having to close my rule window... didn't realize how jarring that was before. |
| recentlyAdded, | ||
| }) => { | ||
| const canAddMore = allowsMultiple || existingInstanceCount === 0; | ||
| const showRecentlyAddedIndicator = recentlyAdded && canAddMore; |
There was a problem hiding this comment.
In relation to the "adding multiple criteria in quick succession" problem, I'm wondering if this might also cause problems being static. It might be worth trying wrapping this in a useEffect to see if anything changes.
There was a problem hiding this comment.
I think moving these into a useEffect wouldn't help (and would actually be less efficient, since I'd have to make state vars and whatnot for them). They're just quick calculations based directly on the props. I think it's fine to re-do them. "adding multiple criteria in quick succession" should be fixed now with changes in c7b4114 and 7981077
…tial timer runs out.
| <div className={css["action-indicators"]}> | ||
| {canAddMore ? ( | ||
| <> | ||
| <i |
There was a problem hiding this comment.
Still related, I was thinking that the icons could also be rendered via a ternary operator. I'm not sure which would actually be faster, but I feel like it might take more time to change the css class and have the css apply to the icon to hide it rather than just saying if this is true, render this, other wise render the other one.
There was a problem hiding this comment.
The reason it's done this way is to allow us to "fade" between the two by changing opacity over time (with css transitions). If we just render one or the other conditionally, the immediate switch can look a little jarring.
|
|
||
| const criteria = useMemo<CatalogCriteria[]>( | ||
| () => getCatalogCriteria(teacherTool), | ||
| [teacherTool.catalog, teacherTool.rubric] | ||
| ); | ||
|
|
||
| function updateRecentlyAddedValue(id: string, value: boolean) { | ||
| setRecentlyAddedIds(prevState => { | ||
| const newState = { ...prevState }; |
There was a problem hiding this comment.
Do you need to make this newState variable? If the updater function is going to modify the previous state, adding/updating the id entry of that state should work, right?
There was a problem hiding this comment.
I'm new to this way of updating state in react so it's possible I'm wrong here, but I think prevState is still considered "state" in React, so it should be treated as immutable, hence the copy before changing.
…ks/teachertool/rework_catalog_modal
What
This change makes it so you can keep the catalog open, add multiple criteria, and interact with other elements on the page without having to close the catalog.
Why
The process of having to open the catalog, check a box (or boxes), then click a button to confirm worked fairly well for single-use criteria, but it feels cumbersome when you want to add multiple instances of the same criteria (like copilot questions or block-exists checks), since you can only add one at a time.
How
I created a new Catalog Overlay that looks similar to a modal, but it is contained within the right side of the screen. When the user clicks on a criteria, it now gets added immediately. If it's a single-use criteria, the button is then disabled and shows a "Max" indicator. Otherwise, a quick "checkmark" appears to acknowledge the addition, then fades out.
Other Notes
Screen Reader Announcer
In order to provide screen reader information when adding criteria, I added a Screen Reader Announcer component which can be used to send messages to the screen reader. This is kind of similar to the FluentUI Announced component.
At some point, we should also hook this up to our toasts so they announce whatever they're displaying, but I didn't want to lump that into this change. Same for the checklist remove buttons.
"Allow Multiple" in Json
We were relying on the presence of parameters to determine if a criteria should allow multiple instances, but this doesn't work very well. Some parameterized criteria (like "At least X comments") should really only allow one, still. The correct thing to do is probably just to make a field in json, but I didn't want to do that in this change. As such, I didn't spend much time on the
criteriaIsSelectablechecks here. That should all be tidied up in the json changes later on.Screenshots
Try It
Upload Target:
https://makecode.microbit.org/app/dcf2a8c4e75daf42961544654658683ecd329fd1-955a2d1201--evalhttps://makecode.microbit.org/app/9f9ad3a8ac1fa0be1f265c4052fa1471087f9901-44ac4a4284--eval