Feat: Async courses added dialogue on Calendar #1336
Conversation
|
@alexespejo I resolved all your comments, lmk if theres anything else ! |
|
|
||
| export default function AsyncCalendarCard() { | ||
| const isMobile = useIsMobile(); | ||
| if (isMobile) return null; |
There was a problem hiding this comment.
issue: unless I'm mistaken, I believe this breaks React's rules of hooks in that it makes the amount of hooks rendered variable between renders
suggestion: move the isMobile check below all of the hooks
if (!visble || isMobile) {
return null;
}| sectionCode: string; | ||
| } | ||
|
|
||
| export default function AsyncCalendarCard() { |
There was a problem hiding this comment.
issue: this is a TBA card, right? Not an async card?
edit: I notice the component seems to oscillate between using TBA, Async, or some combination of both — this should be consistent
| AppStore.off('clearSchedule', handleUpdate); | ||
| AppStore.off('currentScheduleIndexChange', handleUpdate); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
issue: Why do we have a flag that forces a rerender when courses change? My interpretation here is that we want tbaSections to be reactive to the value of AppStore.schedule.getCurrentCourses().
If so, we shouldn't "pipe" reactive events across multiple hooks.
suggestion: we can have this useEffect update the value of tbaSections stored in a useState (this means we don't have to go from "course updated" -> "update trigger" -> "update tbaSections" and instead just say "course updated" -> "update tbaSections"
| const handleScreenshot = () => { | ||
| if (collapsed && visible) { | ||
| setCollapsed(false); | ||
| } | ||
| }; | ||
| AppStore.on('screenshot', handleScreenshot); | ||
| return () => { | ||
| AppStore.off('screenshot', handleScreenshot); | ||
| }; | ||
| }, [collapsed, visible]); |
There was a problem hiding this comment.
issue: this doesn't seem to actually work?
|
|
||
| useEffect(() => { | ||
| const raw = getLocalStorageTempSaveData(); | ||
| if (!raw) return; |
There was a problem hiding this comment.
nit: always use braces
| if (!raw) return; | |
| if (!raw) { | |
| return; | |
| } |
| left: 'auto', | ||
| right: 16, | ||
| zIndex: (theme) => theme.zIndex.drawer - 1, | ||
| width: '22rem', |
There was a problem hiding this comment.
question: why 22 rem? is this specific to some styling of the calendar? is it arbitrary?
| action={ | ||
| <IconButton size="small" onClick={handleToggleCollapse}> | ||
| {collapsed ? <ExpandMore fontSize="small" /> : <ExpandMore fontSize="small" sx={{ transform: 'rotate(180deg)' }} />} | ||
| </IconButton> | ||
| } |
There was a problem hiding this comment.
nit (repeat): formatting
you should check your prettier settings and write an issue if it appears to be a problem with AA and not your configuration
| px: 1, | ||
| width: '100%', | ||
| }} | ||
| action={ |
There was a problem hiding this comment.
issue: the action button looks misaligned/not respecting the alert padding
| </IconButton> | ||
| } | ||
| > | ||
| <AlertTitle sx={{ fontSize: '1em', my: 'auto'}}> |
There was a problem hiding this comment.
question: em? Although em is more correct in some cases, AA tends to use rem in most cases iirc. was this intentional?
| </AlertTitle> | ||
|
|
||
| <Collapse in={!collapsed} timeout="auto" unmountOnExit> | ||
| <Box sx={{ mt: 0.25, py: 0.25 }}> |
There was a problem hiding this comment.
suggestion: is 2 pixels of vertical padding noticeable? seems like we could omit these styles and add a gap property to the container if we want to space out the title and the collapse
Summary
When a user adds Async sections to their schedule, a collapsible dialogue is now added to the bottom right of their calendar.
Alternative to #1310 that's less blaring to the user.
When screenshotted:
Test Plan
Issues
Closes #1181