WIP: implement quality-of-life changes for pre-enroll - #995
Conversation
|
[diff-counting] Significant lines: 64. |
|
Visit the preview URL for this PR (updated for commit 9521050): https://cornelldti-courseplan-dev--pr995-qol-changes-preenrol-5kc7oo53.web.app (expires Wed, 14 May 2025 06:12:33 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933 |
…olor collision there probably is a better approach, but this is what i've come up with so far
| :style="{ | ||
| background: `#${Math.floor(Math.random() * 0xffffff) | ||
| .toString(16) | ||
| .padEnd(6, '0')}`, | ||
| }" |
There was a problem hiding this comment.
@cornell-dti/course-plan What do you think of this approach to ensure that no two tabs have the same color? It seems pretty questionable to me, but this is the best approach I could come up with for now.
There was a problem hiding this comment.
I agree it's a bit questionable - since it’s random, there’s still a chance of collision (and color contrast might be inconsistent too). if we really want to ensure uniqueness and consistent colors we could:
- generate colors from a fixed palette (e.g. COLORS = ['#FFAA00', '#4DA8DA', ...]) and assign them based on tab index or hash of the course ID
- or use a seeded random function so the same course/tab always gets the same color
that said, for a quick temporary solution this totally works - just maybe worth revisiting if it ends up looking chaotic in prod haha
I think this was mentioned in Clemént's design audit for accessibility reasons
nidhi-mylavarapu
left a comment
nidhi-mylavarapu
left a comment
There was a problem hiding this comment.
For the TODOs:
For RequirementFulfillmentGraph.getConnectedRequirementsFromCourse(courseId), we could call this when rendering each course card, and pass the result down as a fulfilledRequirements prop to display somewhere (maybe in a tooltip or under the title?).
for the "master requirements page", maybe we can reuse the existing paginated course list component logic, but instead group/filter by unfulfilled requirements as the top-level sections instead of course categories
| $secondaryGray: #858585; | ||
| $activeGray: #5b676d; | ||
| $disabledGray: #cccccc; | ||
| $disabledGray: #78848a; |
There was a problem hiding this comment.
this changes disabledGray everywhere - was the incorrect hexcode being used originally according to design?
| :style="{ | ||
| background: `#${Math.floor(Math.random() * 0xffffff) | ||
| .toString(16) | ||
| .padEnd(6, '0')}`, | ||
| }" |
There was a problem hiding this comment.
I agree it's a bit questionable - since it’s random, there’s still a chance of collision (and color contrast might be inconsistent too). if we really want to ensure uniqueness and consistent colors we could:
- generate colors from a fixed palette (e.g. COLORS = ['#FFAA00', '#4DA8DA', ...]) and assign them based on tab index or hash of the course ID
- or use a seeded random function so the same course/tab always gets the same color
that said, for a quick temporary solution this totally works - just maybe worth revisiting if it ends up looking chaotic in prod haha
| return requirementsString.substring(0, requirementsString.length - 2); | ||
| } | ||
| return 'testingRequirementsString'; | ||
| // todo: if requirements string is too long, the course card text gets crunched together |
There was a problem hiding this comment.
For the requirementsString length issue - maybe we could:
Limit the number of requirements shown (e.g. 2 or 3), and if there are more, add a +X more indicator (e.g. "MATH, CS, +2 more")
or switch to rendring them as separate badges (like small pill components) which could wrap or scroll more gracefully on smaller course cards
Either way we might also want to set a max-width or text-overflow: ellipsis style if we’re sticking with plain text for now
3 participants
See https://cornelldti.slack.com/archives/C05QHNY8TKL/p1744242627568859
Thanks @ambers7!
TODO:
RequirementFulfillmentGraph.getConnectedRequirementsFromCourse()to add the requirement(s) a course satisfies to its course card in the semester plan