-
Notifications
You must be signed in to change notification settings - Fork 4
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
Ryder PCA Sorting #540
base: master
Are you sure you want to change the base?
Ryder PCA Sorting #540
Conversation
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.
Looks great! Just some nits and small comments
rownum: number; | ||
} | ||
export const AlertHeader = ({ course, rownum }: AlertHeaderProps) => { | ||
return ( |
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.
nit: courseCode
and rowNum
?
@@ -109,6 +195,25 @@ export const ManageAlert = ({ | |||
); | |||
}; | |||
|
|||
let rowNum = 0; |
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.
nit: lets move this to under useStates
|
||
return res; | ||
}, {}); | ||
|
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.
can we generalize this with the other grouping and put it in a shared util file?
rowNum++; | ||
return ( | ||
<> | ||
<AlertHeader course={key} rownum={rowNum} /> |
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.
++rowNum
here, then we can remove rowNum++
above
<AlertItem | ||
key={alert.id} | ||
checked={alertSel[alert.id]} | ||
rownum={rowNum} |
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.
++rowNum
here, then we can removerowNum++
above
nit: rowNum
instead?
@@ -90,6 +91,91 @@ export const ManageAlert = ({ | |||
const [searchTimeout, setSearchTimeout] = useState<number>(); | |||
const [numSelected, setNumSelected] = useState(0); | |||
|
|||
// Alerts for testing |
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.
remove alerts before merge
<> | ||
<AlertHeader course={key} rownum={rowNum} /> | ||
{groupedAlerts[key]?.map?.((alert) => { | ||
console.log(alert); |
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.
remove console log
Wrote sorting logic for manage alerts page so alerts are now grouped by course in lexicographical order. Created banner for course groupings.