Skip to content

feat: Assign colors to events#15298

Merged
CarinaWolli merged 21 commits intocalcom:mainfrom
anikdhabal:issue#7507
Aug 14, 2024
Merged

feat: Assign colors to events#15298
CarinaWolli merged 21 commits intocalcom:mainfrom
anikdhabal:issue#7507

Conversation

@anikdhabal
Copy link
Copy Markdown
Contributor

@anikdhabal anikdhabal commented Jun 2, 2024

What does this PR do?

Screenshot 2024-06-03 232921
Screenshot 2024-06-03 232859

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected)
  • I have added a Docs issue here if this PR makes changes that would require a documentation change
  • I have added or modified automated tests that prove my fix is effective or that my feature works (PRs might be rejected if logical changes are not properly tested)

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 2, 2024

@anikdhabal is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2024

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

@github-actions github-actions Bot added linear Sync Github Issue from community members to Linear.app Low priority Created by Linear-GitHub Sync Medium priority Created by Linear-GitHub Sync ✨ feature New feature or request 🧹 Improvements Improvements to existing features. Mostly UX/UI ❗️ migrations contains migration files labels Jun 2, 2024
@PeerRich PeerRich added this to the v4.2 milestone Jun 3, 2024
@PeerRich PeerRich removed the Medium priority Created by Linear-GitHub Sync label Jun 3, 2024
@PeerRich PeerRich modified the milestones: v4.2, v4.3 Jun 3, 2024
@github-actions github-actions Bot added the Medium priority Created by Linear-GitHub Sync label Jun 3, 2024
@anikdhabal anikdhabal marked this pull request as ready for review June 3, 2024 18:09
@graphite-app graphite-app Bot added the community Created by Linear-GitHub Sync label Jun 3, 2024
@graphite-app graphite-app Bot requested a review from a team June 3, 2024 18:09
@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Jun 3, 2024

Graphite Automations

"Add community label" took an action on this PR • (06/03/24)

1 label was added to this PR based on Keith Williams's automation.

"Add consumer team as reviewer" took an action on this PR • (06/03/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (08/11/24)

1 reviewer was added to this PR based on Keith Williams's automation.

@dosubot dosubot Bot modified the milestones: v4.3, v4.4 Jun 11, 2024
@github-actions
Copy link
Copy Markdown
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions Bot added the Stale label Jun 26, 2024
Comment thread apps/web/components/eventtype/EventAdvancedTab.tsx Outdated
Copy link
Copy Markdown
Member

@Amit91848 Amit91848 left a comment

Choose a reason for hiding this comment

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

Amazing work so far @anikdhabal ! Found a small issue that should be fixed. Can you also fix the merge conflicts.

You select a specific colour for event type and save the event type. Now you toggle the colour settings off and on again, but don't save the event type. The colour reverts to its default value, becoming inconsistent with current state of event type. Ideally it should still be the colour previously saved as we did not make any changes to it. Attached a video for better understanding.

Recording.2024-06-28.230706.mp4

@github-actions github-actions Bot removed the Stale label Jun 29, 2024
@ibex088 ibex088 marked this pull request as draft July 8, 2024 11:27
@github-actions
Copy link
Copy Markdown
Contributor

This PR is being marked as stale due to inactivity.

@anikdhabal
Copy link
Copy Markdown
Contributor Author

anikdhabal commented Jul 31, 2024

This doesn't match the designs in figma https://www.figma.com/design/UajFu4M1APhn2AywAEJkJr/New-Features?node-id=4673-355405&t=ih0GY2rAHb4Wc3wX-.

Hey @Udit-takkar , why do we need to create a different color picker component with limited colors when we already have one? I think the current picker provides more color options. Additionally, creating a new component would make the code larger, and the use case for that color component is only for the event type color picker. Lmk what you think

@Udit-takkar
Copy link
Copy Markdown
Contributor

Hey @Udit-takkar , why do we need to create a different color picker component with limited colors when we already have one?

We can't allow any colour to be selected as we have to make sure it doesn't fail the WCAG Contrast guidelines https://developer.mozilla.org/en-US/docs/Web/Accessibility/Understanding_WCAG/Perceivable/Color_contrast. Just confirm with @ciaranha once about the design and let me know

@anikdhabal
Copy link
Copy Markdown
Contributor Author

Hey @Udit-takkar , why do we need to create a different color picker component with limited colors when we already have one?

We can't allow any colour to be selected as we have to make sure it doesn't fail the WCAG Contrast guidelines https://developer.mozilla.org/en-US/docs/Web/Accessibility/Understanding_WCAG/Perceivable/Color_contrast. Just confirm with @ciaranha once about the design and let me know

Sure @Udit-takkar , and for WCAG contrast guidelines, we can add a check and display a message. I think in our code, we check the contrast for both dark and light modes in some components

@anikdhabal
Copy link
Copy Markdown
Contributor Author

We can't allow any colour to be selected as we have to make sure it doesn't fail the WCAG Contrast guidelines https://developer.mozilla.org/en-US/docs/Web/Accessibility/Understanding_WCAG/Perceivable/Color_contrast. Just confirm with @ciaranha once about the design and let me know

Ciaran confirmed that we can use the color picker that we should have

@anikdhabal anikdhabal requested a review from Udit-takkar August 7, 2024 06:46
Copy link
Copy Markdown
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

CleanShot 2024-08-08 at 11 55 49
Translation not loading

CleanShot 2024-08-08 at 11 57 23
I was able to set this colour and didnt get an error

CleanShot 2024-08-08 at 11 59 50

Created a replit for easier testing of this fn manually.

We may honestly need a light/dark mode colour selector if we're not going for a preset color pallets like in the designs.

@anikdhabal
Copy link
Copy Markdown
Contributor Author

anikdhabal commented Aug 10, 2024

Hey @sean-brydon, the problem is that the way we handle the contrastCheck function is not optimal. The function is guaranteed to return a boolean, but we unnecessarily use it inside a try-catch block in every instance.

Before:-
Screenshot 2024-08-10 133818

After changes:-

Screenshot 2024-08-10 135325

And yes, it would be optimal to add a light and dark mode color selector.

@graphite-app graphite-app Bot requested a review from a team August 11, 2024 11:39
@anikdhabal anikdhabal requested a review from sean-brydon August 12, 2024 06:00
sean-brydon
sean-brydon previously approved these changes Aug 12, 2024
Copy link
Copy Markdown
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

Nice one! Thanks for making them changes - much better behaviour now

@anikdhabal anikdhabal dismissed stale reviews from Udit-takkar and Amit91848 August 12, 2024 10:38

Fixed

@anikdhabal
Copy link
Copy Markdown
Contributor Author

anikdhabal commented Aug 12, 2024

Fixed the conflicts

@anikdhabal anikdhabal requested a review from sean-brydon August 12, 2024 13:10
Copy link
Copy Markdown
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

Tested again - this is a neat feature!

@github-actions
Copy link
Copy Markdown
Contributor

E2E results are ready!

@CarinaWolli CarinaWolli merged commit 528a4fb into calcom:main Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Created by Linear-GitHub Sync community-interns The team responsible for reviewing, testing and shipping low/medium community PRs ✨ feature New feature or request 🧹 Improvements Improvements to existing features. Mostly UX/UI linear Sync Github Issue from community members to Linear.app Low priority Created by Linear-GitHub Sync Medium priority Created by Linear-GitHub Sync ❗️ migrations contains migration files ready-for-e2e

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-2791] Assign colors to events to make them easier to find in the booking overview

7 participants