-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix: Color Mode BoxShadow for cards #2849
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a very clever solution -- don't have any comment right now minus the need for a lint-ignore on line 103! will ✅ once i've tested in mono + monolith
@dreamwasp I haven't done extensive QA but it does fix the homepage issue Screen.Recording.2024-03-18.at.7.16.52.PM.mov |
@dreamwasp I added preview envs to the description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending monorepo errors being cleared
packages/gamut/src/Card/index.tsx
Outdated
outline?: boolean; | ||
mode: ColorModes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seeing test failures in the monorepo here related to this line -maybe happening on initial load before useColorModes has a chance to return the mode? two solutions i can think of are 1. making light the default mode
before if mode doesn't exist yet or 2. making mode not required here (tho having a default fallback might be safer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left notes on mono PR to sync packages there
should be fixed now! Happy to pair on QA as well |
🚀 Styleguide deploy preview ready! |
📬Published Alpha Packages:@codecademy/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA's and looked perfect minus the one small bug with DynamicCardWrapper i pointed out in my comment
boxShadow: `-8px 8px 0 currentColor`, | ||
}, | ||
}, | ||
outline: outlineStyles(mode), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this DynamicCardWrapper variant appears to be broken in dark mode:
jon.mov
i think we only need to theme-sensitive outlineStyles in the variant(s) / CardWrapper
components.
when i apply a variant they work more the way I'd expect them to (and the white variant is perfect). tested locally with the hyper variant:
jon2.mov
Great catch. I believe it is all fixed here. Video of the new functionality With variant=hyper (to be sure it doesn't break variant): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Overview
Redoing #2846
Previews:
Monolith: https://pr-37365-monolith.dev-eks.codecademy.com/
Not sure we use cards on LE: https://tayra.codecademy.com/workspaces/new?PR_ENV=le-pr-5481
Portal: https://tengu.codecademy.com/catalog?PR_ENV=portal-app-pr-5481