Skip to content
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(ui): change red text when instructor not found #483

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aaronthechen
Copy link
Contributor

@aaronthechen aaronthechen commented Jan 5, 2025

Fixes #404.

Updated text when an instructor is not found, changed text size to small, and removed italics.

Before:
"Instructor-specific data is not available for this course, showing course-wide data instead"
After:
"We couldn't find instructor-specific grades, so here are the grades for all (insert course number) sections."


This change is Reviewable

@doprz doprz requested a review from IsaDavRod January 6, 2025 03:19
@doprz doprz added ready-for-review UI/UX-figma PRs that fulfill a task on the UI/UX & Feature Roadmap labels Jan 6, 2025
Copy link
Collaborator

@doprz doprz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend moving the regex logic for formatting the semester value outside of the TSX block and into a computed variable or React.useMemo() (next to the semester variable would be ideal). While the performance improvement might not be significant in most cases, this change enhances code clarity and maintainability. By preprocessing the value once, we avoid unnecessary recalculations on every render, especially if the component re-renders for reasons unrelated to the semester. This approach also makes the TSX cleaner, separating logic from presentation, which improves readability. Although it's a nit, adopting this pattern can make the code easier to extend in the future and reduce potential mistakes when modifying the formatting logic.

Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aaronthechen aaronthechen requested a review from doprz January 7, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review UI/UX-figma PRs that fulfill a task on the UI/UX & Feature Roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change style of "Instructor-specific data is not available for this course, showing course-wide data instead"
3 participants