-
Notifications
You must be signed in to change notification settings - Fork 11
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(ErrorFeedback): new component #883
Conversation
Size stats
|
Accessibility report ℹ️ You can run this locally by executing |
Deploy preview for mistica-web ready! ✅ Preview Built with commit 5f092fd. |
export const ErrorFeedback: React.FC<ErrorFeedbackScreenProps> = ({ | ||
title, | ||
description, | ||
primaryButton, |
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 believe the icon would need to be parametrizable if we are to keep the current VIVO "customization":
platform.getAppBrand() === 'VivoBR' ? (
<IconWarningLight color={skinVars.colors.error} size={60} />
) : (
<IconFileErrorLight color={skinVars.colors.brand} size={60} />
)
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.
hmmm... I don't think so. ErrorFeedbackScreen
doesn't allow you to configure the error icon, I don't se why ErrorFeedback
should allow 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.
we could expose some kind of generic customizable Feedback
as we do with FeedbackScreen
if it's really needed
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.
if the aim is to solve the current issue I raised, yes, we would need that Feedback
if you look at web/src/common/components/full-page-error in webapp, we are using FeedbackScreen instead of ErrorFeedbackScreen because of the fact that the icon needs to be configurable
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 see. It makes sense, but I'm a bit worried we could be over abusing this configurability, we should try to follow the standard ErrorFeedback/ErrorFeedbackScreen design whenever possible unless there is a strong reason. That's the main point of having a Design System. @aweell, what's Design Core opinion on this?
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.
We're worried too we're not following the definition correctly, if we're going to display an error the icon should be consistent. So using errorFeedback / errorFeedbackScreen should be the way to go.
I'm digging into this need with product design. @Winde if you have examples of these cases at hand and can share them, it would be very valuable.
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.
Here's a storybook where it can be seen the icon changing per brand: https://storybook.tuenti.io/?path=/story/account-common-components-fullpageerror--default
Here's the tickets of the last update of this component in webapp:
https://jira.tid.es/browse/PRODUCTDSN-1246
https://jira.tid.es/browse/ACCOUNT-21143
I'm asking @montse for more info on why it was determined a different icon for VIVO.
WEB-1533
Figma: https://www.figma.com/file/LpjgnU1xpmEzQvlXKwAHmn/%F0%9F%94%B8-Feedbacks-Specs?type=design&node-id=2202-130&mode=design&t=rYvgUXRoXd5BNLwo-0