-
Notifications
You must be signed in to change notification settings - Fork 63
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
feat: map page #390
base: main
Are you sure you want to change the base?
feat: map page #390
Conversation
Signed-off-by: doprz <[email protected]>
This reverts commit 87998ce.
Signed-off-by: doprz <[email protected]>
id === selected.start | ||
? '#579D42' | ||
: id === selected.end | ||
? '#D10000' | ||
: node.type === 'building' | ||
? '#BF5700' | ||
: node.type === 'intersection' | ||
? '#9CADB7' | ||
: '#D6D2C400' |
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 this be a function or something instead of /* eslint-disable no-nested-ternary */
at the top of the file?
startId={path.start} | ||
endId={path.end} | ||
graph={graphNodes} | ||
color={path.colors?.primaryColor || '#BF5700'} |
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.
'#BF5700'
(multiple places used in this PR) should be colors.ut.burntorange
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.
Using our theme colors from UnoCSS don't work here so I will import them from src/shared/types/ThemeColors.ts
instead.
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.
(colors.ut.burntorange
is how you'd use it when imported from src/shared/types/ThemeColors.ts
, if it was a class it'd be colors-ut-burntorange
:p)
// eslint-disable-next-line react/no-array-index-key | ||
key={index} |
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.
?
export const DaySelector = ({ selectedDay, onDaySelect }: DaySelectorProps) => ( | ||
<div className='flex gap-2 rounded-md bg-white/90 p-2 shadow-sm'> | ||
{(Object.keys(DAY_MAPPING) as DayCode[]).map(day => { | ||
const color = (selectedDay === day ? 'ut-burntorange' : 'ut-white') as ThemeColor; |
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.
there is no ut-white
, there is ut-offwhite
. I noticed this when changing as ThemeColor
to satisfies ThemeColor
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.
Good catch!
* @param minutes The number of minutes. | ||
* @returns The index value. | ||
*/ | ||
const convertMinutesToIndex = (minutes: number): number => Math.floor((minutes - 420) / 30); |
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.
420 minutes corresponds with 7 a.m. being index 0, but I know of at least one 6 a.m. class
30 assumes no classes start/end on a 15-minute interval
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.
This is the current standard that we have in the calendar so for consistency I used this.
const timeAndLocation = `${time}${location ? ` - ${location.building} ${location.room}` : ''}`; | ||
|
||
const midnightIndex = 1440; | ||
const normalizingTimeFactor = 720; |
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.
if you go about using this constant, at least make it constant at the top, since you use it in convertMinutesToIndex
const dayToNumber = { | ||
Monday: 0, | ||
Tuesday: 1, | ||
Wednesday: 2, | ||
Thursday: 3, | ||
Friday: 4, | ||
Saturday: 5, | ||
Sunday: 6, | ||
} as const satisfies Record<string, number>; |
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.
we have this exact same code in ./src/views/hooks/useFlattenedCourseSchedule.ts
, extract to util?
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.
Yes, that's a good idea. A lot of the things I have right now are for debugging without modifying external components.
<div className='h-full flex flex-none flex-col justify-between pb-5 screenshot:hidden'> | ||
<div className='mb-3 h-full w-fit flex flex-col overflow-auto pb-2 pl-4.5 pr-4 pt-5'> | ||
<CalendarSchedules /> | ||
<Divider orientation='horizontal' size='100%' className='my-5' /> | ||
<ImportantLinks /> | ||
<Divider orientation='horizontal' size='100%' className='my-5' /> | ||
<TeamLinks /> | ||
</div> | ||
<CalendarFooter /> | ||
</div> |
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.
feels like this part should be a shared component of some kind, not just copy/pasted
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.
Yes, that's a good idea. A lot of the things I have right now are for debugging without modifying external components.
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.
yeah it looks weird switching between the calendar tab and this tab because of the inconsistencies, but that'll be fixed when using external stuff
} catch (error) { | ||
console.error('Error rendering path:', error instanceof Error ? error.message : 'Unknown error'); | ||
return null; |
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.
what kind of errors occur/can occur?
edit: oh, PathFindingError
then is there another type of possible error?
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.
Yes, I'm adding custom errors so that when I propagate them up I can handle them separately such as show them via a toast notification.
@@ -0,0 +1,193 @@ | |||
/* eslint-disable max-classes-per-file */ |
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.
it makes sense to export an error class and a logic class from the same file 👍
startId={path.start} | ||
endId={path.end} | ||
graph={graphNodes} | ||
color={path.colors?.primaryColor || '#BF5700'} |
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.
(colors.ut.burntorange
is how you'd use it when imported from src/shared/types/ThemeColors.ts
, if it was a class it'd be colors-ut-burntorange
:p)
{/* Controls and Information */} | ||
<div className='absolute right-8 top-8 flex flex-col gap-4'> | ||
{/* Day Selector */} | ||
<DaySelector selectedDay={selectedDay} onDaySelect={handleDaySelect} /> |
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.
As a user, i'd like the buttons to stay in a consistent place
It's finally time! I've finally ported over one of my beta features from UT Registration Planner to Plus!
Feature Summary:
This change is