feat: add top bar logo for mobile view #1281
Conversation
AlexAndBear
requested review from
Copilot and
kulmann
and removed request for
Copilot
AlexAndBear
force-pushed
the
issues/1265
branch
from
651ef21 to
d61e8c2
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds mobile logo support to the top bar by introducing a responsive logo display mechanism that shows different logos based on screen size. The changes enable customizable logos for mobile viewports while maintaining backward compatibility.
- Added responsive picture element with media queries for mobile logo display
- Extended theme configuration to support optional mobile logo property
- Updated extension SDK with Tailwind CSS configuration and documentation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web-runtime/src/components/Topbar/TopBar.vue | Added responsive picture element to display mobile-specific logo |
| packages/web-pkg/src/composables/piniaStores/theme.ts | Added logoMobile property to theme schema |
| packages/extension-sdk/tailwind.css | New Tailwind CSS configuration for extensions |
| packages/extension-sdk/README.md | Added documentation for Tailwind CSS usage |
| packages/design-system/docs/gettingStarted/tailwindMigration.md | Added note about Tailwind prefix requirements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| <picture> | ||
| <source | ||
| :srcset="currentTheme.logoMobile ? currentTheme.logoMobile : currentTheme.logo" | ||
| media="(max-width: 639px)" |
There was a problem hiding this comment.
The hardcoded breakpoint value 639px should use a CSS custom property or design system token to maintain consistency with other responsive breakpoints in the application.
There was a problem hiding this comment.
not supported by media query
There was a problem hiding this comment.
There is a brief moment before the 639px are reached where the bigger logo starts to be shrinked because we're running out of space horizontally. Just tested around with the bigger breakpoint, 959px also feels good. What do you think? Approving anyway, because this works flawlessly. 🥳
There was a problem hiding this comment.
I think that's fine, we can use the "higher" breakpoint
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
AlexAndBear
force-pushed
the
issues/1265
branch
from
31907a0 to
ce13a9f
Compare
| <picture> | ||
| <source | ||
| :srcset="currentTheme.logoMobile ? currentTheme.logoMobile : currentTheme.logo" | ||
| media="(max-width: 639px)" |
There was a problem hiding this comment.
There is a brief moment before the 639px are reached where the bigger logo starts to be shrinked because we're running out of space horizontally. Just tested around with the bigger breakpoint, 959px also feels good. What do you think? Approving anyway, because this works flawlessly. 🥳
Description
desktop viewport
mobile viewport
Related Issue
How Has This Been Tested?
Types of changes