feat: add possibility to add background color to the suite icon logo #241
Conversation
| width: 36px; | ||
| height: 36px; |
There was a problem hiding this comment.
question Should those be variables?
There was a problem hiding this comment.
this is old code, IMO it should be gone and replaced w the icon size of the icon component (see comment below)
| typeof props.icon !== 'string' ? ( | ||
| props.icon | ||
| ) : ( | ||
| <Icon name={props.icon as keyof typeof Icons} color="brand" size="xxl" /> | ||
| ) |
There was a problem hiding this comment.
question I'm confused why do we need that? 🤔
Are we trying to support both a string and JSX is that it? That's for removing the Icon thing on mP's root page?
Can't see how this change is related to the PR description.
There was a problem hiding this comment.
correct. IMO we should move towards the string with the pre-set size of the icon and color (specially for consistency purposes, since this is only used in the navbar)... but I don't wanna break everything (now)
There was a problem hiding this comment.
right now, we are setting the icon size in the css, but since we have the new size variables we can just use them :)
There was a problem hiding this comment.
huh, if we do go this route, i suggest flipping the if statement, so the true condition comes first
if icon == string, <icon/>, else props.icon
src/components/navigation/GlobalNavigation/GlobalNavigation.tsx
Outdated
Show resolved
Hide resolved
| @@ -50,7 +51,7 @@ export const GlobalNavigation = (props: IGlobalNavigationProps) => { | |||
| <Layout.Sider className="globalNavigation__sider" width={GlobalNavWidth}> | |||
| <Flex vertical justify="space-between" style={{ height: '100%' }}> | |||
| <div> | |||
| <SuiteLogo {...props.logo} /> | |||
| {!props.hideSuiteLogo && <SuiteLogo {...props.logo} />} | |||
| <div className="globalNavigation__divider" /> | |||
There was a problem hiding this comment.
suggestion: I think this divider should also be hidden if suite logo is not showing. it has bottom margin, that's why we have this odd spacing
| @@ -295,6 +290,19 @@ | |||
| cursor: pointer; | |||
| } | |||
|
|
|||
| .globalNavigation__suiteLogo-background { | |||
| background-color: var(--mp-brand-primary-2); | |||
There was a problem hiding this comment.
nit: hmmm, I'd prefer to see semantic names, but it's up to you (I'm not the CSS owner here 😝 )
| background-color: var(--mp-brand-primary-2); | |
| background-color: var(--color-primary-bg-hover); |
There was a problem hiding this comment.
jeje I used this variable to keep consistency w the icons :)
| @@ -11,7 +12,8 @@ export interface IBaseGlobalNavigationItem { | |||
|
|
|||
| export interface IGlobalNavigationLogo extends IBaseGlobalNavigationItem { | |||
| onSuiteLogoClick: () => void | |||
| type?: 'default' | 'background-solid' | |||
| type?: 'default' | 'background-solid' | 'custom-size' | |||
| icon?: ReactElement | keyof typeof Icons | |||
There was a problem hiding this comment.
can we not use ReactNode here? thats what we've been using for most types, do you [or Mr. A.I.] know the difference between the two?
There was a problem hiding this comment.
I changed it to ReactElement bc ReactNode accepts string and I want to enforce to write only the strings that are allowed in the icon component :)
# [1.15.0-icons-integration.2](v1.15.0-icons-integration.1...v1.15.0-icons-integration.2) (2024-05-22) ### Features * add possibility to add background color to the suite icon logo ([#241](#241)) ([d4c206a](d4c206a))
|
🎉 This PR is included in version 1.15.0-icons-integration.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR adds the possibility to add background color to the suite icon logo
story: here