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

[PLAY-1788] Avatar - Dark Mode Audit #4185

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

Conversation

gabbymassaro
Copy link
Contributor

@gabbymassaro gabbymassaro commented Jan 28, 2025

What does this PR do? A clear and concise description with your runway ticket url.
PLAY-1788

This PR addresses the following issues in the Avatar Kit:

  • The "Monogram" variant should use $text_dk_light as the background color
  • The "Icon Circle Component Overlay" variant now follows dark mode standards
  • The "Badge Component Overlay" variant now follows dark mode standards

Out of the scope of the story:

  • There's a bug in light mode around the padding for the Rails "Icon Circle Component Overlay". It was addressed in dark mode as well.

Screenshots: Screenshots to visualize your addition/change
"Monogram" before and after REACT and RAILS:
Screenshot 2025-01-29 at 8 29 18 AM

"Icon Circle Component Overlay" and "Badge Component Overlay" Dark Mode Audit
before and after
REACT and RAILS:
Screenshot 2025-01-29 at 8 30 56 AM

Light Mode Bug "Icon Circle Component Overlay" RAILS ONLY:
Screenshot 2025-01-29 at 8 32 54 AM

How to test? Steps to confirm the desired behavior:

  1. Go to /kits/avatar/rails and /kits/avatar/react
  2. Enable dark mode
  3. Scroll down to the Monogram, Icon Circle Component Overlay and Badge Component Overlay
  4. See dark mode applied to the three components
  5. For the bug: in light mode, go to the Rails - Icon Circle Component Overlay, see that the padding is no longer 24px

Checklist:

  • LABELS Add a label: enhancement, bug, improvement, new kit, deprecated, or breaking. See Changelog & Labels for details.
  • DEPLOY I have added the milano label to show I'm ready for a review.
  • TESTS I have added test coverage to my code.

@gabbymassaro gabbymassaro self-assigned this Jan 28, 2025
@gabbymassaro gabbymassaro added enhancement New Features, Props, & Variants (USED IN CHANGELOG) minor Semver Target labels Jan 28, 2025
@gabbymassaro gabbymassaro marked this pull request as ready for review January 28, 2025 22:32
@gabbymassaro gabbymassaro requested a review from a team as a code owner January 28, 2025 22:32
@gabbymassaro gabbymassaro added milano 20 MAX - Deploy this PR to a review environment via Milano and removed milano 20 MAX - Deploy this PR to a review environment via Milano labels Jan 29, 2025
@gabbymassaro
Copy link
Contributor Author

@nidaqg Side question: do you think these changes require an alpha? I'm still unsure when we should be creating them.

@nidaqg
Copy link
Contributor

nidaqg commented Jan 29, 2025

@nidaqg Side question: do you think these changes require an alpha? I'm still unsure when we should be creating them.

Thats a great question @gabbymassaro ! So generally we decide on a case by case basis. Most of the dark mode stories are small enough changes to not require alpha testing. Things to looks for: If it is a beta kit, or a prop/feature not in use in Nitro, we should not need testing. Your code looks like its only affecting the overlayComponent piece which I believe is not in use in Nitro so should be ok to go without testing (you can check on that piece!)

@nidaqg nidaqg added Code Approved Approved by a Playbook Admin and removed Needs Review labels Jan 29, 2025
@nidaqg nidaqg added Product Approved pending technical review, OK to merge to master Ready for Release merged to master, ready for a versioned released Inactive RC Skip the release candidate process labels Jan 30, 2025
@gabbymassaro gabbymassaro removed the milano 20 MAX - Deploy this PR to a review environment via Milano label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Approved Approved by a Playbook Admin enhancement New Features, Props, & Variants (USED IN CHANGELOG) Inactive RC Skip the release candidate process minor Semver Target Product Approved pending technical review, OK to merge to master Ready for Release merged to master, ready for a versioned released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants