feat: calendar custom time blocks#847
feat: calendar custom time blocks#847Uth24 wants to merge 39 commits intoLonghorn-Developers:mainfrom
Conversation
|
Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
|
1 similar comment
|
Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
|
|
Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
|
1 similar comment
|
Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
|
b801582 to
f5103d9
Compare
68ceaeb to
0ae04f4
Compare
doprz
left a comment
There was a problem hiding this comment.
@doprz made 1 comment.
Reviewable status: 0 of 17 files reviewed, 1 unresolved discussion (waiting on arunbagavathiannan, beastgwert, DereC4, lesliewlooi, margaret-ca, Razboy20, Samathingamajig, songhannahha-hub, Uth24, and vincent-situ).
a discussion (no related file):
Please rebase this PR to include the latest changes from main. I can see that you are still using eslint in this PR which was removed in favor of Biome. Also while rebasing please fix the merge conflicts.
doprz
left a comment
There was a problem hiding this comment.
@doprz made 9 comments.
Reviewable status: 0 of 17 files reviewed, 9 unresolved discussions (waiting on arunbagavathiannan, beastgwert, DereC4, lesliewlooi, margaret-ca, Razboy20, Samathingamajig, songhannahha-hub, Uth24, and vincent-situ).
src/views/components/calendar/AddCustomTimeBlockPopover.tsx line 32 at r1 (raw file):
} return h * 60 + m; }
TODO: I would extract this into a util file and add tests.
Code quote:
function parseTimeToMinutes(value: string): number | null {
const parts = value.split(':').map(Number);
const h = parts[0];
const m = parts[1];
if (h === undefined || m === undefined || !Number.isFinite(h) || !Number.isFinite(m)) {
return null;
}
return h * 60 + m;
}src/views/components/calendar/EditCustomTimeBlockOverlay.tsx line 31 at r1 (raw file):
} return h * 60 + m; }
TODO: Same here, extract into util file and add tests.
Code quote:
function minutesToTimeInputValue(totalMinutes: number): string {
const h = Math.floor(totalMinutes / 60);
const m = totalMinutes % 60;
return `${String(h).padStart(2, '0')}:${String(m).padStart(2, '0')}`;
}
function parseTimeToMinutes(value: string): number | null {
const parts = value.split(':').map(Number);
const h = parts[0];
const m = parts[1];
if (h === undefined || m === undefined || !Number.isFinite(h) || !Number.isFinite(m)) {
return null;
}
return h * 60 + m;
}src/views/components/calendar/EditCustomTimeBlockOverlay.tsx line 61 at r1 (raw file):
setSyncAcrossAllSchedules(block.syncAcrossAllSchedules); setHighlightCatalogConflicts(block.highlightCatalogConflicts); }, [block]);
Not a huge fan of hardcoding the start and end time. iirc we have this value stored somewhere so let's use that instead.
Code quote:
const [startTime, setStartTime] = useState(() =>
block.allDay ? '08:00' : minutesToTimeInputValue(block.startTime)
);
const [endTime, setEndTime] = useState(() => (block.allDay ? '09:00' : minutesToTimeInputValue(block.endTime)));
const [allDay, setAllDay] = useState(!!block.allDay);
const [syncAcrossAllSchedules, setSyncAcrossAllSchedules] = useState(block.syncAcrossAllSchedules);
const [highlightCatalogConflicts, setHighlightCatalogConflicts] = useState(block.highlightCatalogConflicts);
useEffect(() => {
setTitle(block.title);
setDays(block.days);
setStartTime(block.allDay ? '08:00' : minutesToTimeInputValue(block.startTime));
setEndTime(block.allDay ? '09:00' : minutesToTimeInputValue(block.endTime));
setAllDay(!!block.allDay);
setSyncAcrossAllSchedules(block.syncAcrossAllSchedules);
setHighlightCatalogConflicts(block.highlightCatalogConflicts);
}, [block]);src/views/components/calendar/EditCustomTimeBlockOverlay.tsx line 108 at r1 (raw file):
syncAcrossAllSchedules, highlightCatalogConflicts, });
nit. Use satisfies keyword for this. Good use of spread operator.
Code quote:
await upsertCustomTimeBlock({
...block,
title: title.trim(),
days: sortDaysByWeek(days),
startTime: allDay ? 0 : startM,
endTime: allDay ? 1440 : endM,
allDay,
syncAcrossAllSchedules,
highlightCatalogConflicts,
});src/views/components/injected/TableRow/TableRow.tsx line 61 at r1 (raw file):
return () => { UserScheduleStore.unsubscribe(unsub); };
This will need to be refactored as @Razboy20 's recent PR refactored stores. Please see my original comment of rebasing the PR.
Code quote:
UserScheduleStore.get('customTimeBlocks').then(v => setCustomTimeBlocks(v ?? []));
const unsub = UserScheduleStore.subscribe('customTimeBlocks', ({ newValue }) => {
setCustomTimeBlocks(newValue ?? []);
});
return () => {
UserScheduleStore.unsubscribe(unsub);
};src/views/hooks/useFlattenedCourseSchedule.ts line 85 at r1 (raw file):
export const GRID_DEFAULT_END = 1260; /**
See previous comment about hardcoding values and let's check where else we use a start and end time.
Code quote:
* 8:00 AM latest start time aka DEFAULT, in minutes
*/
export const GRID_DEFAULT_START = 480;
/**
* 9:00 PM earliest end time aka DEFAULT, in minutes
*/
export const GRID_DEFAULT_END = 1260;
/**src/views/hooks/useFlattenedCourseSchedule.ts line 108 at r1 (raw file):
const allMeetings = [...activeSchedule.courses.flatMap(c => c.schedule.meetings), ...customMeetingsForBounds]; // go through every meeting we have and finds minimum start time with starting best so far being GRID_DEFAULT_START_MINUTES
nit. Start comment with uppercase. Please reword to be a bit more concise
src/views/hooks/useFlattenedCourseSchedule.ts line 112 at r1 (raw file):
const rawGridStartMinutes = allMeetings.reduce((earliest, current) => { if (current.days.includes(DAY_MAP.S) || current.startTime >= 1440) { // keep accumulator value unchanged, go to next iteration
nit. Comment isn't needed as we know this about reduce and accumulators.
src/views/hooks/useFlattenedCourseSchedule.ts line 315 at r1 (raw file):
endIndex: convertMinutesToIndex(normalizedEndTime, gridStartMinutes), }, });
FYI, it would be good to use the satisfies keyword here.
Code quote:
cells.push({
block,
displayTitle: block.title,
displayTime: timeLine,
async: false,
calendarGridPoint: {
dayIndex,
startIndex: convertMinutesToIndex(normalizedStartTime, gridStartMinutes),
endIndex: convertMinutesToIndex(normalizedEndTime, gridStartMinutes),
},
});
This change is