fix: use local manager logos#31
Open
piwel wants to merge 2 commits into
Open
Conversation
Reviewer's GuideReplaces external Evolution logo and favicon URLs with local SVG assets, centralizes brand logo paths, and introduces a resolved theme concept so dark/light logos react correctly to system theme changes. Sequence diagram for logo selection based on resolved themesequenceDiagram
participant Browser
participant ThemeProvider
participant DocumentRoot as documentElement
participant Login
participant brand
Browser->>ThemeProvider: load ThemeProvider
ThemeProvider->>ThemeProvider: useState(theme, resolvedTheme)
ThemeProvider->>DocumentRoot: remove light,dark classes
alt theme is system
ThemeProvider->>Browser: window.matchMedia("(prefers-color-scheme: dark)")
Browser-->>ThemeProvider: mediaQuery
ThemeProvider->>ThemeProvider: applySystemTheme()
ThemeProvider->>DocumentRoot: add dark or light class
ThemeProvider->>ThemeProvider: setResolvedTheme(systemTheme)
Browser-->>ThemeProvider: mediaQuery change events
ThemeProvider->>ThemeProvider: applySystemTheme() on change
else theme is explicit
ThemeProvider->>DocumentRoot: add theme class
ThemeProvider->>ThemeProvider: setResolvedTheme(theme)
end
Browser->>Login: render Login
Login->>ThemeProvider: useTheme()
ThemeProvider-->>Login: { resolvedTheme }
Login->>brand: EVOLUTION_LOGO_DARK_URL or EVOLUTION_LOGO_LIGHT_URL
brand-->>Login: logoSrc
Login->>Browser: render img src=logoSrc
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
piwel
force-pushed
the
codex/local-manager-logos
branch
from
f1bd003 to
1ffe2c4
Compare
piwel
force-pushed
the
codex/local-manager-logos
branch
from
1ffe2c4 to
f358b57
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new brand URLs are hardcoded as absolute
/assets/...paths, which will break if the app is ever served from a non-rootbasepath; consider using static imports ornew URL(..., import.meta.url)(orimport.meta.env.BASE_URL) so asset resolution respects the configured base URL. - The
resolvedThemestate is initialized to'light', so consumers will briefly see the light logo even if the system prefers dark until the effect runs; if that matters for UX, you could lazily derive the initialresolvedThemefrommatchMediawhentheme === 'system'(guarded forwindowavailability).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new brand URLs are hardcoded as absolute `/assets/...` paths, which will break if the app is ever served from a non-root `base` path; consider using static imports or `new URL(..., import.meta.url)` (or `import.meta.env.BASE_URL`) so asset resolution respects the configured base URL.
- The `resolvedTheme` state is initialized to `'light'`, so consumers will briefly see the light logo even if the system prefers dark until the effect runs; if that matters for UX, you could lazily derive the initial `resolvedTheme` from `matchMedia` when `theme === 'system'` (guarded for `window` availability).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
Addressed in
Validation: |
Author
|
Note for reviewers: I noticed after opening this PR that #30 already addresses the same broken branding asset issue. Sorry, I missed it earlier. The overlap is intentional only in the problem being fixed: both PRs remove the dead The main differences from #30 are:
If maintainers prefer #30 as the base, I can close this PR or rework these differences into a follow-up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
📋 Description
Fixes Evolution Manager branding assets by replacing broken remote Evolution logo/favicon URLs with local SVG files.
What changed:
https://evolution-api.com/files/evo/*.svglogo references with bundled local SVG assets.systemand the OS is dark.src/lib/brand.ts.🔗 Related Issues
No related issue found.
🧪 Type of Change
🧪 Testing
Test Environment
Test Cases
Test Instructions
npm run build./assets/images/*.svgpaths.Validation performed:
npm_config_cache=/private/tmp/npm-cache-evolution-manager npm run build📸 Screenshots
Not included.
✅ Checklist
Code Quality
npm run lintand fixed any issuesTesting
Documentation
Internationalization
Breaking Changes
🔄 Migration Guide
Not applicable.
📝 Additional Notes
The resolved theme value keeps the logo selection correct when the selected theme is
system; it also listens for OS color scheme changes.🤝 Reviewer Guidelines
Focus Areas
Testing Checklist for Reviewers