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.

Sign up for GitHub

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

[NO-JIRA] Migrate Floating Notification component to TS #3221

Merged
merged 8 commits into from
Feb 8, 2024
Merged

[NO-JIRA] Migrate Floating Notification component to TS #3221

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
918091a
migrate Floating Notification component to TS
Feb 6, 2024
e52ea4f
rollback examples.js and stories.js
Feb 7, 2024
7974d03
add ts-expect-error
Feb 7, 2024
22be9f8
update snapshots
Feb 7, 2024
85e236f
improve types
Feb 7, 2024
079f271
remove unnecessary nulls
Feb 7, 2024
f42b21b
fix snapshots change by mistake
Feb 8, 2024
d5e973f
Merge branch 'main' into NO-JIRA_Migrating_floating_notification_to_TS
danielmartinezskyscanner Feb 8, 2024
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
0 ...-component-floating-notification/index.js → ...-component-floating-notification/index.ts
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions ...ation/src/BpkFloatingNotification-test.js → ...tion/src/BpkFloatingNotification-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import { fireEvent, render } from '@testing-library/react';

import { cssModules } from '../../bpk-react-utils';
// @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`.
import BpkIconHeart from '../../bpk-component-icon/sm/heart';

import BpkFloatingNotification from './BpkFloatingNotification';
Expand Down
45 changes: 45 additions & 0 deletions packages/bpk-component-floating-notification/src/BpkFloatingNotification.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Backpack - Skyscanner's Design System
*
* Copyright 2016 Skyscanner Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import type { ReactNode, ReactElement, MouseEvent } from 'react';

export type Props = {
animateOnEnter?: boolean;
animateOnExit?: boolean;
className?: string;
ctaText?: string;
hideAfter?: number;
icon?: () => ReactElement;
onClick?: (e: MouseEvent) => void;
onExit?: () => void;
text: string;
[rest: string]: any;
};
declare const BpkFloatingNotification: ({
animateOnEnter,
animateOnExit,
className,
ctaText,
hideAfter,
icon,
onClick,
onExit,
text,
...rest
}: Props) => JSX.Element;
export default BpkFloatingNotification;
67 changes: 23 additions & 44 deletions ...tification/src/BpkFloatingNotification.js → ...ification/src/BpkFloatingNotification.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,58 +17,60 @@
*/
/* @flow strict */

import PropTypes from 'prop-types';
import { useEffect, useState, ReactElement } from 'react';
import { CSSTransition } from 'react-transition-group';
import type { MouseEvent, FunctionComponent } from 'react';
import { useEffect, useState } from 'react';
// @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`.
import CSSTransition from 'react-transition-group/CSSTransition';

import BpkAriaLive from '../../bpk-component-aria-live';
import {BUTTON_TYPES, BpkButtonV2} from '../../bpk-component-button';
import { BUTTON_TYPES, BpkButtonV2 } from '../../bpk-component-button';
import BpkText, { TEXT_STYLES } from '../../bpk-component-text';
import { cssModules } from '../../bpk-react-utils';

import STYLES from './BpkFloatingNotification.module.scss';

const getClassName = cssModules(STYLES);

type Props = {
animateOnEnter: ?boolean,
animateOnExit: ?boolean,
className: ?string,
ctaText: ?string,
export type Props = {
animateOnEnter?: boolean;
animateOnExit?: boolean;
className?: string;
ctaText?: string;
/**
* This prop controls the amount of time that the notification stays visible before the exit animation begins.
* The default value is 4 seconds (4000 milliseconds).
*/
hideAfter: ?number,
icon: ?() => ReactElement,
onClick: ?() => void,
hideAfter?: number;
icon?: FunctionComponent;
onClick?: (e: MouseEvent<HTMLButtonElement>) => void;
/**
* Execute a function after the component has finished the exit animation.
*/
onExit: ?() => void,
text: string,
onExit?: () => void;
text: string;
[rest: string]: any; // Inexact rest. See decisions/inexact-rest.md
};

const BpkFloatingNotification = (props: Props) => {
const [showMessage, setShowMessage] = useState(true);

const {
animateOnEnter,
animateOnExit,
className,
ctaText,
hideAfter,
animateOnEnter = true,
animateOnExit = true,
className = null,
ctaText = null,
hideAfter = 4000,
icon: Icon,
onClick,
onExit,
onExit = null,
text,
...rest
} = props;

const classNames = getClassName('bpk-floating-notification', className);

useEffect(() => {
let timer;
let timer: ReturnType<typeof setTimeout>;
if (hideAfter) {
timer = setTimeout(() => setShowMessage(false), hideAfter);
}
Expand Down Expand Up @@ -118,27 +120,4 @@ const BpkFloatingNotification = (props: Props) => {
);
};

BpkFloatingNotification.propTypes = {
animateOnEnter: PropTypes.bool,
animateOnExit: PropTypes.bool,
className: PropTypes.string,
ctaText: PropTypes.string,
hideAfter: PropTypes.number,
icon: PropTypes.ReactElement,
onClick: PropTypes.func,
onExit: PropTypes.func,
text: PropTypes.string.isRequired,
};

BpkFloatingNotification.defaultProps = {
animateOnEnter: true,
animateOnExit: true,
className: null,
ctaText: null,
hideAfter: 4000,
icon: null,
onClick: null,
onExit: null,
};

export default BpkFloatingNotification;
10 changes: 5 additions & 5 deletions ...ts__/BpkFloatingNotification-test.js.snap → ...s__/BpkFloatingNotification-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`BpkFloatingNotification should render correctly 1`] = `
<DocumentFragment>
<div
class="bpk-floating-notification bpk-floating-notification--appear bpk-floating-notification--appear-active"
class="bpk-floating-notification"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why the classnames have changed here? Looks like it might not be appearing anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting 👀 The UI behaviour was as expected but, for some reason, the tests were rendering the component without those classes. After some debugging, I found out that it was because of this change:

import { CSSTransition } from 'react-transition-group';
___________________________________________________________________

import CSSTransition from 'react-transition-group/CSSTransition';

It might be something related to babel and jest... Anyway, I already fixed it and rollbacked the snapshot changes 🙂

>
<p
class="bpk-text bpk-text--body-default bpk-floating-notification__text"
Expand All @@ -24,7 +24,7 @@ exports[`BpkFloatingNotification should render correctly 1`] = `
exports[`BpkFloatingNotification should render correctly with CTA text 1`] = `
<DocumentFragment>
<div
class="bpk-floating-notification bpk-floating-notification--appear bpk-floating-notification--appear-active"
class="bpk-floating-notification"
>
<p
class="bpk-text bpk-text--body-default bpk-floating-notification__text"
Expand All @@ -51,7 +51,7 @@ exports[`BpkFloatingNotification should render correctly with CTA text 1`] = `
exports[`BpkFloatingNotification should render correctly with icon prop 1`] = `
<DocumentFragment>
<div
class="bpk-floating-notification bpk-floating-notification--appear bpk-floating-notification--appear-active"
class="bpk-floating-notification"
>
<div
class="bpk-floating-notification__icon"
Expand Down Expand Up @@ -86,7 +86,7 @@ exports[`BpkFloatingNotification should render correctly with icon prop 1`] = `
exports[`BpkFloatingNotification should support arbitrary props 1`] = `
<DocumentFragment>
<div
class="bpk-floating-notification bpk-floating-notification--appear bpk-floating-notification--appear-active"
class="bpk-floating-notification"
testid="123"
>
<p
Expand All @@ -108,7 +108,7 @@ exports[`BpkFloatingNotification should support arbitrary props 1`] = `
exports[`BpkFloatingNotification should support custom class names 1`] = `
<DocumentFragment>
<div
class="bpk-floating-notification custom-classname bpk-floating-notification--appear bpk-floating-notification--appear-active"
class="bpk-floating-notification custom-classname"
>
<p
class="bpk-text bpk-text--body-default bpk-floating-notification__text"
Expand Down
3 changes: 2 additions & 1 deletion ...ng-notification/src/accessibility-test.js → ...g-notification/src/accessibility-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import { render } from '@testing-library/react';
import { axe } from 'jest-axe';

// @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`.
import BpkIconHeart from '../../bpk-component-icon/sm/heart';

import BpkFloatingNotification from './BpkFloatingNotification';
Expand All @@ -30,7 +31,7 @@ const props = {

describe('BpkFloatingNotification accessibility tests', () => {
it('should not have programmatically-detectable accessibility issues', async () => {
const { container } = render(<BpkFloatingNotification />);
const { container } = render(<BpkFloatingNotification {...props} />);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to clarify the addition of the ...props here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text prop is required in the component, calling it without it would fail.
I added the whole props object since it only has the text property.

const results = await axe(container);
expect(results).toHaveNoViolations();
});
Expand Down
Loading