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

allow choosing of no color #4008

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kaze-droid
Copy link
Contributor

Context

Resolves #3999

Implementation

Provides an additional option of no color for the color picker by introducing an additional colorIndex for transparent cells

noColor

Copy link

vercel bot commented Feb 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2025 4:02pm
nusmods-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2025 4:02pm

Copy link

vercel bot commented Feb 3, 2025

@kaze-droid is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 55.14%. Comparing base (988c6fd) to head (ed15af8).
Report is 85 commits behind head on master.

Files with missing lines Patch % Lines
website/src/views/timetable/TimetableCell.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4008      +/-   ##
==========================================
+ Coverage   54.52%   55.14%   +0.61%     
==========================================
  Files         274      276       +2     
  Lines        6076     6327     +251     
  Branches     1455     1548      +93     
==========================================
+ Hits         3313     3489     +176     
- Misses       2763     2838      +75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@jloh02 jloh02 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Looks great just have a few comments for minor aesthetic and quality improvements

@@ -51,7 +51,7 @@ const ColorPicker = memo<Props>((props) => {
className={classnames(styles.palette, { [styles.isClosed]: !isOpen })}
{...getMenuProps()}
>
{_.range(NUM_DIFFERENT_COLORS).map((index: ColorIndex) => (
{_.range(TOTAL_COLORS).map((index: ColorIndex) => (
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Since the 8 colors currently form a 4x2 grid, the 9th cell makes it look kinda strange. Should we try to make the current color change to transparent color on click (e.g. if the current course color is blue, clicking on the originally blue cell should disable the color?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow choosing of "No Color" for color picker
2 participants