Course Card Popover #207
Course Card Popover #207
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
sanjana-singhania
force-pushed
the
206-course-card-popup
branch
from
21079be to
98aa5e7
Compare
sanjana-singhania
force-pushed
the
206-course-card-popup
branch
from
98aa5e7 to
4e72719
Compare
sanjana-singhania
requested review from
ItsEricSun,
Sarah-Zhang1,
aarowz and
jpnortonwastaken
mehallhm
left a comment
mehallhm
left a comment
There was a problem hiding this comment.
a couple of other remarks:
- I think I am going to go through and remove all the default tailwindcss colors. that would strong arm people to use our design system since there is still stuff coming through the cracks (I SEE YOU AI USAGE... I ALWAYS KNOW [not targeted at you just the teams in general])
- I think I need to do an audit / PR with some refactoring changes... I have been more generous in these PRs since there has not been clear code style guidelines yet. Next sem I already have some docs being drafted, but I think I am going to walk the codebase and find things that can just be improved. you guys are going hard on the features, but there is some little stuff that I would like to fix
neither of those are immediate concerns, but will be implemented slowly throughout break
|
|
||
| {/* Content */} | ||
| <div className="pt-2 pb-4 px-6"> | ||
| <table className="w-full"> |
There was a problem hiding this comment.
you should not be using table here. Table is for tabular content... and should not be used for layout. There is a long list of reasons why which W3 officially suggests, but this should just be divs / spans frankly. No need to complicate it! table should frankly be one the last things to ever reach for, its usage is highly specific
There was a problem hiding this comment.
going back and being unbrutal (didn't mean to come across as aggressive, just being very straightforward). I see why you did it! I think we can optimize this... and also prob pull it out of the scheduler here soon since I think I would like to use this pattern elsewhere too
| ) | ||
| } | ||
|
|
||
| const formatMeetingDays = (meetingDays: number[]): React.ReactNode => { |
There was a problem hiding this comment.
we really gotta abstract this
There was a problem hiding this comment.
this and also formatTime... is there anywhere specific you want utils like that to be? i was thinking to abstract it but didn't know where to keep it
There was a problem hiding this comment.
let me think about that. leave it for now and we can circle back once we find a better home. i am resituating some things now with the monorepo so ill have a better idea then
| @@ -0,0 +1,142 @@ | |||
| "use client" | |||
|
|
|||
| import React from "react" | |||
There was a problem hiding this comment.
do not import all of react, just the parts you need
| return ( | ||
| <Popover> | ||
| <PopoverTrigger asChild>{children}</PopoverTrigger> | ||
| <PopoverContent className="w-[360px] p-0 border-gray-200 shadow-lg" align="start" side="right" sideOffset={8}> |
There was a problem hiding this comment.
a lot of these colors have to be fixed... see main comment too
| return ( | ||
| <div className="grid w-full grid-cols-6"> | ||
| <div className="col-span-1 w-full"> | ||
| <FilterPanel |
There was a problem hiding this comment.
if there was whitespace here a linter did not run...
| className={` | ||
| px-3 py-2 rounded-lg border whitespace-nowrap font-bold flex items-center gap-2 text-neu8 | ||
| ${currentCourseGroupIndex === index | ||
| ? "border-neu3 bg-white" |
There was a problem hiding this comment.
more default colors that need to be fixed
| px-3 py-2 rounded-lg border whitespace-nowrap font-bold flex items-center gap-2 text-neu8 | ||
| ${currentCourseGroupIndex === index | ||
| ? "border-neu3 bg-white" | ||
| ${currentCourseGroupIndex === index |
There was a problem hiding this comment.
you really would have a better time using the cn util... it is made of merging classnames together...
Closes #206
Displays a popover for the section on click.
Added building and room number to meeting time.
Removes the duplicate classes in filter bar (oops) and also removes "clear filtering" to stay up to date with figma designs.