Skip to content

Teacher Tool: Make Catalog Non-Modal #9983

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

Merged
merged 33 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
1d30be1
Create RightPanel for containing the catalog overlay and the project …
thsparks Apr 18, 2024
7dd00ec
CatalogModal -> CatalogOverlay
thsparks Apr 18, 2024
b4acd89
In progress...
thsparks Apr 18, 2024
11f3acc
Base working version
thsparks Apr 18, 2024
29387ec
Catalog no longer uses modal stuff in state
thsparks Apr 18, 2024
699c840
Hide add criteria button when catalog is open
thsparks Apr 18, 2024
0041831
Disable used criteria instead of hiding them
thsparks Apr 19, 2024
a36b22e
Some spacing adjustments and a to do
thsparks Apr 19, 2024
5592bbb
Fix close button formatting
thsparks Apr 19, 2024
ce1fcd2
Experimenting with check boxes / plus-minus
thsparks Apr 19, 2024
277ab88
Merge branch 'master' of https://github.com/microsoft/pxt into thspar…
thsparks Apr 22, 2024
3ff90bf
Use count instead of boolean for existing instances
thsparks Apr 22, 2024
e7e6ba4
Use plus and max
thsparks Apr 22, 2024
a66aa7d
Remove logic for removing as well as adding
thsparks Apr 22, 2024
193a1d0
Remove unused imports
thsparks Apr 22, 2024
395704f
Prettier
thsparks Apr 22, 2024
4ce1e3c
Fix lfs
thsparks Apr 22, 2024
fbd64ae
Do not scroll the header
thsparks Apr 22, 2024
30e8b0a
Remove RightPanel, put CatalogOverlay directly inside ProjectWorkspace
thsparks Apr 22, 2024
9c7d149
Move component definitions so they aren't nested. The nesting was cau…
thsparks Apr 22, 2024
ebf4ead
Undo unnecessary changes
thsparks Apr 22, 2024
d516c35
Add an "recently added indicator"
thsparks Apr 22, 2024
15380cc
Make recently added indicator fade
thsparks Apr 22, 2024
ade227b
Stop action indicators from shrinking
thsparks Apr 22, 2024
d3ab60f
Add screen reader announcer
thsparks Apr 22, 2024
c03b67c
Undo unnecessary change
thsparks Apr 22, 2024
08e001f
Use FocusTrap to set focus when the overlay first appears
thsparks Apr 22, 2024
aa50399
Fix horizontal scroll bar
thsparks Apr 23, 2024
96bdc6e
Fix non-rounded corners when scrolling
thsparks Apr 23, 2024
d796dd8
Prettier
thsparks Apr 23, 2024
c7b4114
Fix inconsistency with recently added ids, when added in quick succes…
thsparks Apr 23, 2024
7981077
Reset "recently added" timers when button is clicked again before ini…
thsparks Apr 23, 2024
e628527
Merge branch 'master' of https://github.com/microsoft/pxt into thspar…
thsparks Apr 24, 2024
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
4 changes: 2 additions & 2 deletions teachertool/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import { logDebug } from "./services/loggingService";
import { HeaderBar } from "./components/HeaderBar";
import { MainPanel } from "./components/MainPanel";
import { Toasts } from "./components/Toasts";
import { CatalogModal } from "./components/CatalogModal";
import { showToast } from "./transforms/showToast";
import { loadCatalogAsync } from "./transforms/loadCatalogAsync";
import { loadValidatorPlansAsync } from "./transforms/loadValidatorPlansAsync";
import { tryLoadLastActiveRubricAsync } from "./transforms/tryLoadLastActiveRubricAsync";
import { ImportRubricModal } from "./components/ImportRubricModal";
import { ConfirmationModal } from "./components/ConfirmationModal";
import { BlockPickerModal } from "./components/BlockPickerModal";
import { ScreenReaderAnnouncer } from "./components/ScreenReaderAnnouncer";

export const App = () => {
const { state, dispatch } = useContext(AppStateContext);
Expand Down Expand Up @@ -57,11 +57,11 @@ export const App = () => {
<>
<HeaderBar />
<MainPanel />
<CatalogModal />
<ImportRubricModal />
<ConfirmationModal />
<BlockPickerModal />
<Toasts />
<ScreenReaderAnnouncer />
</>
);
};
17 changes: 5 additions & 12 deletions teachertool/src/components/AddCriteriaButton.tsx
Original file line number Diff line number Diff line change
@@ -1,29 +1,22 @@
import { getSelectableCatalogCriteria } from "../state/helpers";
import { Button } from "react-common/components/controls/Button";
import { showModal } from "../transforms/showModal";
import { AppStateContext } from "../state/appStateContext";
import { useContext, useMemo } from "react";
import { useContext } from "react";
import { classList } from "react-common/components/util";
import { Strings } from "../constants";
import { CatalogDisplayOptions } from "../types/modalOptions";
import { setCatalogOpen } from "../transforms/setCatalogOpen";

interface IProps {}

export const AddCriteriaButton: React.FC<IProps> = ({}) => {
const { state: teacherTool } = useContext(AppStateContext);

const hasAvailableCriteria = useMemo<boolean>(
() => getSelectableCatalogCriteria(teacherTool).length > 0,
[teacherTool.catalog, teacherTool.rubric]
);
return (
return !teacherTool.catalogOpen ? (
<Button
className={classList("inline", "outline-button")}
label={Strings.AddCriteria}
onClick={() => showModal({ modal: "catalog-display" } as CatalogDisplayOptions)}
onClick={() => setCatalogOpen(true)}
title={Strings.AddCriteria}
leftIcon="fas fa-plus-circle"
disabled={!hasAvailableCriteria}
/>
);
) : null;
};
85 changes: 0 additions & 85 deletions teachertool/src/components/CatalogModal.tsx

This file was deleted.

166 changes: 166 additions & 0 deletions teachertool/src/components/CatalogOverlay.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
import { useContext, useMemo, useState } from "react";
import { AppStateContext } from "../state/appStateContext";
import { addCriteriaToRubric } from "../transforms/addCriteriaToRubric";
import { CatalogCriteria } from "../types/criteria";
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 { 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 css from "./styling/CatalogOverlay.module.scss";

interface CatalogHeaderProps {
onClose: () => void;
}
const CatalogHeader: React.FC<CatalogHeaderProps> = ({ onClose }) => {
return (
<div className={css["header"]}>
<span className={css["title"]}>{Strings.SelectCriteriaDescription}</span>
<Button
className={css["close-button"]}
rightIcon="fas fa-times-circle"
onClick={onClose}
title={Strings.Close}
/>
</div>
);
};

interface CatalogItemLabelProps {
catalogCriteria: CatalogCriteria;
allowsMultiple: boolean;
existingInstanceCount: number;
recentlyAdded: boolean;
}
const CatalogItemLabel: React.FC<CatalogItemLabelProps> = ({
catalogCriteria,
allowsMultiple,
existingInstanceCount,
recentlyAdded,
}) => {
const canAddMore = allowsMultiple || existingInstanceCount === 0;
const showRecentlyAddedIndicator = recentlyAdded && canAddMore;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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

return (
<div className={css["catalog-item-label"]}>
<div className={css["action-indicators"]}>
{canAddMore ? (
<>
<i
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

className={classList(
"fas fa-check",
css["recently-added-indicator"],
showRecentlyAddedIndicator ? undefined : css["hide-indicator"]
)}
title={lf("Added!")}
/>
<i
className={classList(
"fas fa-plus",
showRecentlyAddedIndicator ? css["hide-indicator"] : undefined
)}
title={Strings.AddToChecklist}
/>
</>
) : (
<span className={css["max-label"]}>{Strings.Max}</span>
)}
</div>
<ReadOnlyCriteriaDisplay catalogCriteria={catalogCriteria} showDescription={true} />
</div>
);
};

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.rubric]
);

function updateRecentlyAddedValue(id: string, value: NodeJS.Timeout | undefined) {
setRecentlyAddedIds(prevState => {
const newState = { ...prevState };
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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'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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right, makes sense!

if (value) {
newState[id] = value;
} else {
delete newState[id];
}
return newState;
});
}

function onItemClicked(c: CatalogCriteria) {
addCriteriaToRubric([c.id]);

// Set a timeout to remove the recently added indicator
// and save it in the state.
if (recentlyAddedIds[c.id]) {
clearTimeout(recentlyAddedIds[c.id]);
}
const timeoutId = setTimeout(() => {
updateRecentlyAddedValue(c.id, undefined);
}, recentlyAddedWindowMs);
updateRecentlyAddedValue(c.id, timeoutId);

announceToScreenReader(lf("Added '{0}' to checklist.", getReadableCriteriaTemplate(c)));
}

return (
<div className={css["catalog-list"]}>
{criteria.map(c => {
const allowsMultiple = c.params !== undefined && c.params.length !== 0; // TODO add a json flag for this (MaxCount or AllowMultiple)
const existingInstanceCount = teacherTool.rubric.criteria.filter(
i => i.catalogCriteriaId === c.id
).length;
return (
c.template && (
<Button
id={`criteria_${c.id}`}
title={getReadableCriteriaTemplate(c)}
key={c.id}
className={css["catalog-item"]}
label={
<CatalogItemLabel
catalogCriteria={c}
allowsMultiple={allowsMultiple}
existingInstanceCount={existingInstanceCount}
recentlyAdded={recentlyAddedIds[c.id] !== undefined}
/>
}
onClick={() => onItemClicked(c)}
disabled={!allowsMultiple && existingInstanceCount > 0}
/>
)
);
})}
</div>
);
};

interface CatalogOverlayProps {}
export const CatalogOverlay: React.FC<CatalogOverlayProps> = ({}) => {
const { state: teacherTool } = useContext(AppStateContext);

function closeOverlay() {
setCatalogOpen(false);
}

return teacherTool.catalogOpen ? (
<FocusTrap onEscape={() => {}}>
<div className={css["catalog-overlay"]}>
<div className={css["catalog-content-container"]}>
<CatalogHeader onClose={closeOverlay} />
<CatalogList />
</div>
</div>
</FocusTrap>
) : null;
};
2 changes: 2 additions & 0 deletions teachertool/src/components/ProjectWorkspace.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getSafeProjectName } from "../state/helpers";
import { Toolbar } from "./Toolbar";
import { ShareLinkInput } from "./ShareLinkInput";
import { MakeCodeFrame } from "./MakecodeFrame";
import { CatalogOverlay } from "./CatalogOverlay";

const ProjectName: React.FC = () => {
const { state: teacherTool } = useContext(AppStateContext);
Expand All @@ -16,6 +17,7 @@ const ProjectName: React.FC = () => {
export const ProjectWorkspace: React.FC = () => {
return (
<div className={css.panel}>
<CatalogOverlay />
<Toolbar center={<ProjectName />} />
<ShareLinkInput />
<MakeCodeFrame />
Expand Down
17 changes: 17 additions & 0 deletions teachertool/src/components/ScreenReaderAnnouncer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { useContext } from "react";
import { AppStateContext } from "../state/appStateContext";
import css from "./styling/ActionAnnouncer.module.scss";

export interface ScreenReaderAnnouncerProps {}
export const ScreenReaderAnnouncer: React.FC<ScreenReaderAnnouncerProps> = () => {
const { state: teacherTool } = useContext(AppStateContext);
return (
<>
{teacherTool.screenReaderAnnouncement && (
<div className={css["sr-only"]} aria-live="polite">
{teacherTool.screenReaderAnnouncement}
</div>
)}
</>
);
};
13 changes: 13 additions & 0 deletions teachertool/src/components/styling/ActionAnnouncer.module.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Hide content but keep component around for screen readers
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
border: 0;
white-space: nowrap;
border-width: 0;
}
14 changes: 0 additions & 14 deletions teachertool/src/components/styling/CatalogModal.module.scss

This file was deleted.

Loading