Skip to content
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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions playroom/snippets.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ const feedbackSnippets: Array<Snippet> = [
'ErrorFeedbackScreen',
'InfoFeedbackScreen',
'SuccessFeedback',
'ErrorFeedback',
].map((name) => ({
group: 'Feedbacks',
name,
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
30 changes: 30 additions & 0 deletions src/__screenshot_tests__/error-feedback-screenshot-test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import {openStoryPage, screen} from '../test-utils';
import {MOVISTAR_SKIN, O2_SKIN} from '../skins/constants';

import type {Device} from '../test-utils';

const testableSkins = [MOVISTAR_SKIN, O2_SKIN] as const;
const testableDevices: Array<Device> = ['MOBILE_IOS', 'DESKTOP'];

const cases: Array<[(typeof testableSkins)[number], Device]> = [];
for (const skin of testableSkins) {
for (const device of testableDevices) {
cases.push([skin, device]);
}
}

test.each(cases)('ErrorFeedback on %s and %s', async (skin, device) => {
await openStoryPage({
id: `patterns-feedback-errorfeedback--error-feedback-story`,
skin,
device,
args: {
errorReference: 'E-1234',
},
});

const errorFeedback = await screen.findByTestId('error-feedback');

const image = await errorFeedback.screenshot();
expect(image).toMatchImageSnapshot();
});
50 changes: 50 additions & 0 deletions src/__stories__/error-feedback-story.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import * as React from 'react';
import {ErrorFeedback} from '../feedback';
import {ButtonPrimary, ButtonSecondary} from '../button';
import ResponsiveLayout from '../responsive-layout';

export default {
title: 'Patterns/Feedback/ErrorFeedback',
parameters: {
fullScreen: true,
},
};

type Args = {
title: string;
description: string;
multipleParagraphs: boolean;
errorReference: string;
withButtons: boolean;
};

export const ErrorFeedbackStory: StoryComponent<Args> = ({
title,
description,
multipleParagraphs,
withButtons,
errorReference,
}) => (
<ResponsiveLayout>
<ErrorFeedback
dataAttributes={{testid: 'error-feedback'}}
title={title}
description={multipleParagraphs ? [description, 'paragraph 2', 'paragraph 3'] : description}
primaryButton={
withButtons ? <ButtonPrimary onPress={() => {}}>Action1</ButtonPrimary> : undefined
}
secondaryButton={
withButtons ? <ButtonSecondary onPress={() => {}}>Action2</ButtonSecondary> : undefined
}
errorReference={errorReference}
/>
</ResponsiveLayout>
);
ErrorFeedbackStory.storyName = 'ErrorFeedback';
ErrorFeedbackStory.args = {
title: 'Title',
description: 'Description',
multipleParagraphs: false,
errorReference: '',
withButtons: true,
};
86 changes: 62 additions & 24 deletions src/feedback.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import ButtonGroup from './button-group';
import {vars} from './skins/skin-contract.css';
import * as styles from './feedback.css';
import IconSuccessVivoNew from './icons/icon-success-vivo-new';
import {getPrefixedDataAttributes} from './utils/dom';

import type {Theme} from './theme';
import type {DataAttributes, IconProps} from './utils/types';
Expand Down Expand Up @@ -144,14 +145,10 @@ const renderFeedbackBody = (
);
};

const renderInlineFeedbackBody = (
feedbackBody: React.ReactNode,
buttons: ButtonGroupProps,
isTabletOrSmaller: boolean
) => {
const renderInlineFeedbackBody = (feedbackBody: React.ReactNode, buttons: ButtonGroupProps) => {
const hasButtons = checkHasButtons(buttons);
return (
<Stack space={isTabletOrSmaller ? 24 : 40}>
<Stack space={{mobile: 24, desktop: 40}}>
{feedbackBody}
{hasButtons && <ButtonGroup {...buttons} />}
</Stack>
Expand Down Expand Up @@ -257,15 +254,11 @@ export const FeedbackScreen: React.FC<FeedbackScreenProps> = ({
animateText && areAnimationsSupported(platformOverrides),
appear
);
const inlineFeedbackBody = renderInlineFeedbackBody(
feedbackBody,
{
primaryButton,
secondaryButton,
link,
},
isTabletOrSmaller
);
const inlineFeedbackBody = renderInlineFeedbackBody(feedbackBody, {
primaryButton,
secondaryButton,
link,
});

if (!isTabletOrSmaller && unstable_inlineInDesktop) {
return inlineFeedbackBody;
Expand Down Expand Up @@ -432,15 +425,11 @@ export const SuccessFeedback: React.FC<AssetFeedbackProps> = ({
areAnimationsSupported(platformOverrides),
appear
);
const inlineFeedbackBody = renderInlineFeedbackBody(
feedbackBody,
{
primaryButton,
secondaryButton,
link,
},
isTabletOrSmaller
);
const inlineFeedbackBody = renderInlineFeedbackBody(feedbackBody, {
primaryButton,
secondaryButton,
link,
});

return isTabletOrSmaller ? (
<ResponsiveLayout isInverse>
Expand All @@ -459,3 +448,52 @@ export const SuccessFeedback: React.FC<AssetFeedbackProps> = ({
})
);
};

export const ErrorFeedback: React.FC<ErrorFeedbackScreenProps> = ({
title,
description,
primaryButton,
Copy link
Contributor

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} />
                )

Copy link
Contributor Author

@atabel atabel Sep 14, 2023

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

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

secondaryButton,
link,
errorReference,
dataAttributes,
}) => {
useHapticFeedback('error');
const {isTabletOrSmaller} = useScreenSize();
const {platformOverrides} = useTheme();

const appear = useAppearStatus();

const icon = <IconError size="100%" />;
const feedbackBody = renderFeedbackBody(
{
icon,
title,
description,
extra: errorReference ? (
<Text2 color={vars.colors.textSecondary} regular>
{errorReference}
</Text2>
) : undefined,
},
areAnimationsSupported(platformOverrides),
appear
);
const inlineFeedbackBody = renderInlineFeedbackBody(feedbackBody, {
primaryButton,
secondaryButton,
link,
});

return isTabletOrSmaller ? (
<div {...getPrefixedDataAttributes({'component-name': 'ErrorFeedback', ...dataAttributes})}>
{inlineFeedbackBody}
</div>
) : (
renderFeedbackInDesktop({
isInverse: false,
inlineFeedbackBody,
dataAttributes: {'component-name': 'ErrorFeedback', ...dataAttributes},
})
);
};
1 change: 1 addition & 0 deletions src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export {
InfoFeedbackScreen,
SuccessFeedbackScreen,
SuccessFeedback,
ErrorFeedback,
} from './feedback';
export {default as IconButton} from './icon-button';
export {default as Popover} from './popover';
Expand Down