-
Notifications
You must be signed in to change notification settings - Fork 105
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
chore(Cross): [IOAPPX-448] Add missing components to the Design System section, remove legacy ones + Change ListItemMessage
component API
#6541
base: master
Are you sure you want to change the base?
Conversation
Affected stories
|
Jira Pull Request LinkThis Pull Request refers to the following Jira issue IOAPPX-448 |
/* Component Types */ | ||
|
||
type BaseBannerErrorStateProps = WithTestID<{ | ||
icon?: IOIcons; | ||
icon?: BannerErrorStateIcons; |
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.
Do we really need to limit the icons here? What do you think about making the entire IOIcons set available instead? It would certainly provide more flexibility for potential future use cases.
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.
@Hantex9 It certainly provides more flexibility, but the component already has a specific job (Error...
), which is to signal states related to errors or warnings. For other purposes we already have other components like Banner
or Alert
. I can't see BannerErrorState
being used with a smiley icon, for example.
So I think it's better to limit the choice for now. If we need some flexibility in the future, things will change.
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.
It makes sense, and I understand your point about the component having a specific purpose. However, limiting the icons to a predefined set could become a constraint in the future if new error-related icons are introduced or required for additional use cases.
This approach might lead to an increased boilerplate if we need to update the component later to include more icons. Allowing the entire IOIcons
set would future-proof the component and reduce the need for maintenance in case requirements evolve. What do you think?
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.
limiting the icons to a predefined set could become a constraint in the future if new error-related icons are introduced or required for additional use cases
I consider adding this level of friction to be a feature rather than a bug, but I also understand your point about new error-related icons. I'll restore the IOIcons
generic type 👍🏻
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.
@Hantex9 Addressed here → 1ddd447
(#6541)
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.
LGTM!
Short description
This PR updates the Design System section with missing components. Checking them in isolation is essential to avoid regression during development.
List of changes proposed in this pull request
BannerErrorState
to theDSAdvice
pageListItemSearchInstitution
to theDSListItems
pageCgnModuleDiscount
toModuleCgnDiscount
, refactor it and move it toDSModules
fromDSListItems
DSColors
because CGN doesn't use them anymoreMessageListItem
→ListItemMessage
DoubleAvatar
toAvatarDouble
(to keep the same convention as the others) and add it to theDSLogos
pagebadgeText
andbadgeVariant
props with the singletag
propCustomPressableListItemBase
by integrating it intoListItemMessage
ListItemMessage
possible combinations toDSListItems
for testing purposesPreview
The new
ListItemMessage
section in the List Items pageHow to test
Go to the Design System section and check the update pages