-
Notifications
You must be signed in to change notification settings - Fork 596
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
Changes from all commits
1d30be1
7dd00ec
b4acd89
11f3acc
29387ec
699c840
0041831
a36b22e
5592bbb
ce1fcd2
277ab88
3ff90bf
e7e6ba4
a66aa7d
193a1d0
395704f
4ce1e3c
fbd64ae
30e8b0a
9c7d149
ebf4ead
d516c35
15380cc
ade227b
d3ab60f
c03b67c
08e001f
aa50399
96bdc6e
d796dd8
c7b4114
7981077
e628527
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
}; |
This file was deleted.
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; | ||
return ( | ||
<div className={css["catalog-item-label"]}> | ||
<div className={css["action-indicators"]}> | ||
{canAddMore ? ( | ||
<> | ||
<i | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; |
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> | ||
)} | ||
</> | ||
); | ||
}; |
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; | ||
} |
This file was deleted.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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