-
Notifications
You must be signed in to change notification settings - Fork 0
light mode #468
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
light mode #468
Conversation
This reverts commit 8ca946e.
WalkthroughThis pull request updates dependency versions and adds a new dependency in the package file. It overhauls the CSS custom properties for improved modularity, revises dark mode handling by shifting from static HTML to dynamic JavaScript, and adjusts styling across various Svelte components. New components for a logo and mode switching are introduced, and layout as well as chart color values are updated. Finally, a dedicated success color scale is added to the Tailwind configuration for enhanced theming. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MS as ModeSwitcher
participant LS as localStorage
participant AT as App HTML Script
participant HTML as <html> Element
U->>MS: Click mode toggle
MS->>LS: Save mode (light/dark)
Note over MS,LS: Mode stored in localStorage
U->>AT: Load page/script executes
AT->>LS: Retrieve stored mode
LS-->>AT: Return mode value
alt dark mode active
AT->>HTML: Add "dark" class
else
AT->>HTML: No "dark" class added
end
Possibly related PRs
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
# Conflicts: # frontend/package-lock.json # frontend/package.json
--warning-800: 44 100% 55%; | ||
--warning-900: 45 100% 60%; | ||
--warning-950: 40 100% 86%; | ||
--success-50: 159 46% 11%; |
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.
I ended up moving the warning colors to :root so they are shared between light and dark mode for now.
<head> | ||
<meta charset="utf-8" /> | ||
<link rel="icon" href="%sveltekit.assets%/logo.svg" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
<script> |
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.
Helps prevent a flicker when the page initially loads
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.
I do something similar in LayerStack/Svelte UX (little trickier with multiple dark themes). Nice work!
@@ -0,0 +1,37 @@ | |||
<script lang="ts"> |
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.
Created a Logo.svelte for easier changing of fill
@sean-zlai Should be ready for review |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/app.css (1)
42-170
: Light Mode Props Updated. New custom properties for background, muted, neutrals, primary, warning, and success are well-organized. Please double-check that the duplicate values for--warning-50
and--warning-200
are intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
frontend/package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (16)
frontend/package.json
(1 hunks)frontend/src/app.css
(3 hunks)frontend/src/app.html
(1 hunks)frontend/src/lib/components/ExpandableCell.svelte
(1 hunks)frontend/src/lib/components/Logo.svelte
(1 hunks)frontend/src/lib/components/ModeSwitcher.svelte
(1 hunks)frontend/src/lib/components/NavigationBar.svelte
(2 hunks)frontend/src/lib/components/NavigationSlider.svelte
(1 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte
(1 hunks)frontend/src/lib/components/ui/badge/index.ts
(1 hunks)frontend/src/lib/components/ui/tabs/tabs-trigger.svelte
(1 hunks)frontend/src/routes/+layout.svelte
(3 hunks)frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte
(4 hunks)frontend/src/routes/[conf]/[name]/observability/summary/+page.svelte
(1 hunks)frontend/src/routes/[conf]/[name]/overview/+page.svelte
(3 hunks)frontend/tailwind.config.js
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/src/lib/components/charts/PercentileLineChart.svelte
- frontend/src/routes/[conf]/[name]/observability/summary/+page.svelte
- frontend/src/lib/components/ExpandableCell.svelte
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: frontend_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (27)
frontend/src/lib/components/NavigationSlider.svelte (1)
9-9
: Updated background for light/dark modeChanged from hardcoded
bg-white
to conditionalbg-black dark:bg-white
, aligns with light mode theme.frontend/src/lib/components/ui/badge/index.ts (1)
16-16
: Replaced hardcoded colors with theme variablesUsing
bg-success-50 text-success-500
instead of hex values improves maintainability.frontend/tailwind.config.js (1)
81-81
: Added success color scaleSuccess color scale supports the badge component update and enhances theming capabilities.
frontend/src/lib/components/Logo.svelte (1)
1-37
: New SVG Logo component with theme supportLogo now uses
currentColor
which adapts to light/dark mode automatically.SVG implementation will scale better than image and allows for dynamic color changes.
frontend/src/app.html (1)
7-15
: Dynamic theme management implementation looks good.Clean approach that prevents flickering by checking preferences before page render.
frontend/src/lib/components/NavigationBar.svelte (2)
43-43
: Logo component import looks good.Clean SVG-based component adoption.
109-109
: Proper implementation of Logo component.Text-foreground class ensures proper theming.
frontend/src/lib/components/ui/tabs/tabs-trigger.svelte (1)
18-18
: Color scheme adjustment for better light/dark mode contrast.Adjusted primary colors (800→600) for light mode while preserving dark mode values.
frontend/src/lib/components/ModeSwitcher.svelte (3)
1-8
: Good component structure and imports.Clean organization with proper icon and utility imports.
10-21
: Well-implemented theme toggle button.Nice transition effects with accessible labeling.
22-26
: Good dropdown implementation for theme selection.Comprehensive options with proper alignment.
frontend/package.json (3)
71-71
: Layer chart version bump ✓Library update for
layerchart
from^1.0.3
to^1.0.5
.
73-73
: New dependency for theme management ✓Addition of
mode-watcher
package for light/dark theme handling.
75-75
: Svelte UX version bump ✓Minor version update for
svelte-ux
from^1.0.0
to^1.0.1
.frontend/src/routes/+layout.svelte (4)
9-10
: Theme mode components imported ✓Added imports for theme management.
23-23
: Mode watcher added ✓
ModeWatcher
component correctly placed at the root level.
32-32
: Background color updated for light/dark mode ✓Updated background classes to support both themes.
41-44
: Layout reorganized for theme switcher ✓Clean restructuring to accommodate theme toggle.
frontend/src/routes/[conf]/[name]/overview/+page.svelte (3)
229-229
: Group background updated for light/dark mode ✓Added light mode background with dark mode fallback.
490-490
: Added background color to inspection panel ✓Applied neutral background to improve readability.
509-509
: Adjusted highlight color for better contrast ✓Modified HSL values for improved visibility.
frontend/src/routes/[conf]/[name]/observability/drift/+page.svelte (5)
148-150
: Updated sticky header for light/dark mode ✓Applied theme-compatible background colors.
351-351
: Updated baseline chart color ✓Changed to a more theme-appropriate blue.
356-356
: Updated current chart color ✓Changed to a more theme-appropriate green.
366-369
: Enhanced chart styling ✓Added missing tooltip config and removed bar strokes.
411-411
: Updated pie chart color range ✓Synced colors with bar chart palette.
frontend/src/app.css (1)
214-224
: Dark Mode Success Colors Adjusted. The updated success tokens look good—verify they align with the intended design.
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.
Really great work!
<head> | ||
<meta charset="utf-8" /> | ||
<link rel="icon" href="%sveltekit.assets%/logo.svg" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
<script> |
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.
I do something similar in LayerStack/Svelte UX (little trickier with multiple dark themes). Nice work!
<Button builders={[builder]} variant="outline" size="icon"> | ||
<IconSun | ||
class="h-[1.2rem] w-[1.2rem] rotate-0 scale-100 transition-all dark:-rotate-90 dark:scale-0" | ||
/> | ||
<IconMoon | ||
class="absolute h-[1.2rem] w-[1.2rem] rotate-90 scale-0 transition-all dark:rotate-0 dark:scale-100" | ||
/> | ||
<span class="sr-only">Toggle theme</span> | ||
</Button> |
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 feels a touch big to me. I see it's ~36x36px. On shadcn-svelte's docs it's 32x32. Maybe make it 1rem
instead of 1.2rem
?
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.
Done! I changed the size of the button to 2rem (since our base size is 16px, 2rem comes out to 32px w/h)
Summary
light-mode.mp4
Checklist
Summary by CodeRabbit
New Features
Style