-
Notifications
You must be signed in to change notification settings - Fork 174
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: move theme options out of labs #2340
base: qa
Are you sure you want to change the base?
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
Preview - Build & Deploy Images✅ Build images 🕒 Last deployed: Feb 28, 2025 at 07:59 PM UTC |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## qa #2340 +/- ##
==========================================
+ Coverage 89.93% 89.94% +0.01%
==========================================
Files 380 380
Lines 83654 83720 +66
==========================================
+ Hits 75231 75306 +75
+ Misses 8423 8414 -9 ☔ View full report in Codecov by Sentry. |
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.
can you please fix the drag/drop modal dark mode
Screencast.from.2025-02-28.04-48-33.webm
Nice, good catch @AyushAgrawal-A2 — it's a straightforward fix, swapped some hard-coded colors to theme variables. Screenshots: |
<EmptyGridMessage /> | ||
<Search /> |
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.
Moved these just to try and put all these elements in the same place. They were in QuadraticGrid before, but ultimately render in the same place. So I'm sticking them here where everything else is.
@@ -148,7 +148,7 @@ export const SuggestionDropDown = () => { | |||
return ( | |||
<div | |||
className={cn( | |||
'border.gray-300 pointer-events-auto absolute cursor-pointer overflow-y-auto border bg-white text-gray-500', | |||
'pointer-events-auto absolute cursor-pointer overflow-y-auto rounded-sm border border-border bg-background text-muted-foreground', |
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.
@@ -37,7 +35,6 @@ export default function QuadraticGrid() { | |||
return ( | |||
<div | |||
ref={containerRef} | |||
className="dark-mode-hack" |
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.
Moved this from the parent because it was inverting everything under it. This cause HTML-rendered elements like the empty sheet message to invert to white in dark mode:
Instead, we now apply dark-mode-hack
to any individual element that should be inverted. Hopefully over time we can strip these out one-by-one and apply proper theme colors.
@@ -45,6 +45,8 @@ export const Following = () => { | |||
</div> | |||
<div | |||
style={{ | |||
top: 0, | |||
left: 0, |
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.
dark-mode-hack
which uses filter changes stacking contexts in CSS, so this now needs a reference for its positioning since we removed it from the parent.
<div className="fixed bottom-16 left-8 z-50 h-[92px] w-[403px] select-none border border-slate-200 bg-white pb-2 pl-4 pr-4 pt-2 tracking-tight shadow-[0_2px_5px_0px_rgba(0,0,0,0.15)]"> | ||
<div className="flex justify-between"> | ||
<div className="absolute bottom-4 left-4 z-50 w-96 select-none rounded border border-border bg-background pb-2 pl-4 pr-4 pt-2 tracking-tight shadow-lg"> | ||
<div className="mb-2 flex items-center justify-between"> |
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.
Resolves #2283
Notes on implementation: