fix: sidebar styles refactoring and sidebar light theme #1208
Conversation
3em
commented
Mar 7, 2025
3em
commented
- fix new app sidebar with design
- add new light theme to app sidebar
✅ Deploy Preview for harness-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for harness-xd-review ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -39,6 +83,8 @@ export const AppViewWrapper: FC<PropsWithChildren<AppViewWrapperProps>> = ({ | |||
| setShowSettingsMenu(current => !current) | |||
| }, []) | |||
|
|
|||
| console.log(pinnedMenu) | |||
There was a problem hiding this comment.
| console.log(pinnedMenu) |
| className="text-sidebar-foreground-6 data-[highlighted]:bg-sidebar-background-2 data-[highlighted]:text-sidebar-foreground-1" | ||
| onSelect={handlePin} | ||
| > | ||
| <Text size={2} truncate color="inherit"> | ||
| {t('component:navbar.pin')} |
There was a problem hiding this comment.
I know these weren't introduced in this PR, but can we add the fallback values to the calls to t just to make sure they render nicely when we're mocking the translations stuff?
| {t('component:navbar.pin')} | |
| {t('component:navbar.pin', 'Pin')} |
| @@ -47,48 +47,48 @@ export function CommandPaletteWrapper() { | |||
| label: 'Search repositories...', | |||
| key: PageKey.REPOSITORIES, | |||
| shortcut: ['⇧', 'R'], | |||
There was a problem hiding this comment.
I know, also not part of this PR, but these things marked up as being shift-R seems really weird 🤔
| @@ -47,48 +47,48 @@ export function CommandPaletteWrapper() { | |||
| label: 'Search repositories...', | |||
| key: PageKey.REPOSITORIES, | |||
| shortcut: ['⇧', 'R'], | |||
| icon: () => <Icon name="repositories" className="!w-[14px] !h-[14px] text-icons-1" /> | |||
| icon: () => <Icon name="repositories" className="!h-[14px] !w-[14px] text-icons-1" /> | |||
There was a problem hiding this comment.
I wonder why we can't use the standard width/height/size props instead of passing custom classes?
| icon: () => <Icon name="repositories" className="!h-[14px] !w-[14px] text-icons-1" /> | |
| icon: () => <Icon name="repositories" size={14} className="text-icons-1" /> |
| <Text | ||
| className={cn('z-10 w-full truncate leading-4 text-foreground-4 duration-0 ease-in-out', { | ||
| 'text-sidebar-foreground-4': isMainNav | ||
| })} | ||
| size={1} | ||
| truncate |
There was a problem hiding this comment.
Do we need both the truncate prop as well as the truncate class?
| React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.ScrollAreaScrollbar> | ||
| >(({ className, orientation = 'vertical', ...props }, ref) => ( | ||
| React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.ScrollAreaScrollbar> & { | ||
| scrollBarClassName?: string |
There was a problem hiding this comment.
Since this is a ScrollBar it's kinda weird having className and scrollBarClassName.
| className={cn( | ||
| 'text-icons-4 hover:text-icons-2 focus:ring-0 focus-visible:outline-none', | ||
| closeClassName | ||
| )} | ||
| variant="custom" | ||
| size="icon" | ||
| onClick={handleClose} |
There was a problem hiding this comment.
Do we still need this if the SheetPrimitive.Close is using asChild?
| onClick={handleClose} |
| @@ -8,8 +8,9 @@ const ScrollArea = React.forwardRef< | |||
| React.ElementRef<typeof ScrollAreaPrimitive.Root>, | |||
| React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.Root> & { | |||
| viewportClassName?: string | |||
| scrollBarClassName?: string | |||
There was a problem hiding this comment.
This kinda annoys me. "Scrollbar" is a single word, not two. And this doesn't get applied to the scrollbar, but rather the ScrollAreaThumb
| @@ -1750,13 +1790,27 @@ | |||
| /* Sidebar */ | |||
| --canary-sidebar-background-01: var(--canary-black); | |||
| --canary-sidebar-background-02: hsla(var(--canary-grey-50) / 0.1); | |||
| --canary-sidebar-background-03: hsla(var(--canary-grey-50) / 0); | |||
There was a problem hiding this comment.
So this just becomes transparent?
