-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: icons integration #239
Conversation
…rium into feat/icons-suites
# [1.15.0-icons-integration.1](v1.14.0...v1.15.0-icons-integration.1) (2024-05-21) ### Features * icon sizes ([#238](#238)) ([e093faa](e093faa))
# [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))
Co-authored-by: jared-dickman <zenador15@gmail.com>
# [1.15.0-icons-integration.3](v1.15.0-icons-integration.2...v1.15.0-icons-integration.3) (2024-05-23) ### Bug Fixes * Align button content to the center ([#243](#243)) ([ca0901d](ca0901d))
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.
Good work! Left some suggestions.
src/components/navigation/GlobalNavigation/GlobalNavigation.tsx
Outdated
Show resolved
Hide resolved
@@ -11,6 +12,8 @@ export interface IBaseGlobalNavigationItem { | |||
|
|||
export interface IGlobalNavigationLogo extends IBaseGlobalNavigationItem { | |||
onSuiteLogoClick: () => void | |||
type?: 'default' | 'background-solid' | 'custom-size' | |||
icon?: ReactElement | keyof typeof Icons |
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.
Not sure about ReactElement
here, seems like it's good enough for the options we have (?) so let's go with that but we should look for a consistent usage of React types across the codebase.
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.
ReactNode accepts a string and I want to enforce to use just one of the icons's name :)
Here is the difference detailed
export interface IButtonProps extends AntButtonProps {} | ||
|
||
export interface IButtonProps extends AntButtonProps { | ||
variant?: 'with-icon' |
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.
So this is still confusing for me. Let me try to explain:
We have an with-icon
variant, which by only reading it implies we should use it whenever we have a button with an icon. But that's not the case, right? Because there's a case right now where we have an icon (old icons) and we don't use the variant. The variant was introduced to fix new icons only.
Conclusion: This variant is not actually being used for what its name describes.
So I was thinking, we should either have a better name OR add some comments to explain. But then something else came to mind. If we plan to discontinue this once we get rid of all the old icons, should we make a @deprecated
comment? It's weird for something to be born already deprecated but I think it makes sense in this case.
Let me know how you feel about it.
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 would even go as far as renaming it as well. If we only need to use the variant to fix the problem for new icons while we don't get rid of the old ones, we could call it something like with-new-icon
and leave a @deprecated
comment explaining the differences between old/new icons. Once we swap all icons, we fix the styling problem and get rid of the variant alltogether.
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 :)
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 PR is included in version 1.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR is an integration branch used for testing before merging it to main.
The main topic is Icons.
It handles the alignment of the text and icon inside the button:
PR 1
PR 2
The possibility to have an icon with a violet background and adds some missing svgs
PR 3
And it adds icon sizes
PR 4
As a result, in this realese users will be able to
with-icon
inside the buttonadd a background color to the icon using the property
type="background-solid"
and choose different sizes of the icons with some new sizes like
'xxxxl', 'xxxl', 'xxl', 'xl', 'lg', 'md', 'sm', 'xs'