[MAT-1205] [BpkSkeleton] Add new component - bpk-component-skeleton #3337

felix-luo-sc · 2024-04-07T10:30:47Z

Add new component - bpk-component-skeleton

Figma

https://www.figma.com/file/irZ3YBx8vOm16ICkAr7mB3/Backpack-Components?type=design&node-id=18542-18065&mode=design&t=uAl9OPzRHO5BX3Wy-0

This component can be viewed here: https://backpack.github.io/storybook-prs/3337/?path=/docs/bpk-component-skeleton--documentation

Remember to include the following changes:

Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
README.md (If you have created a new component)
Component README.md
Tests
Storybook examples created/updated
Type declaration (.d.ts) files updated
For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

github-actions · 2024-04-07T10:33:35Z

	Warnings
⚠️	Package source files (e.g. `packages/package-name/src/Component.js`) were updated, but snapshots weren't. Have you checked that the tests still pass?
⚠️	Package source files (e.g. `packages/package-name/src/Component.tsx`) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against a7b8095

github-actions · 2024-04-07T10:33:52Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-08T07:39:59Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-08T07:41:55Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

* add test file * add accessibility test --------- Co-authored-by: Felix luo <felix.luo@skyscanner.net>

github-actions · 2024-04-09T07:01:55Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

Co-authored-by: Felix luo <felix.luo@skyscanner.net>

github-actions · 2024-04-09T07:35:13Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

Co-authored-by: Felix luo <felix.luo@skyscanner.net>

github-actions · 2024-04-09T08:30:10Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-09T09:01:00Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

examples/bpk-component-skeleton/stories.ts

lynn-sc · 2024-04-10T07:35:35Z

packages/bpk-component-skeleton/README.md

+  <BpkImageSkeleton
+    style={IMAGE_SKELETON_STYLE.rounded}
+    size={{width: '7rem', height: '6rem'}}
+    ariaLabel="loading"
+  />
+
+  <BpkBodyTextSkeleton
+    size={SIZE_TYPES.large}
+    ariaLabel="loading"
+    className="custom-class"
+  />
+
+  <BpkCircleSkeleton
+    size={SIZE_TYPES.small}
+    ariaLabel="loading"
+    className="custom-class"
+  />
+
+   <BpkHeadlineSkeleton
+    size={SIZE_TYPES.small}
+    ariaLabel="loading"
+    className="custom-class"
+  />


There should be a div wrapper here. What do you think?

Yes, agree with you. Updated.

lynn-sc · 2024-04-10T07:40:31Z

packages/bpk-component-skeleton/src/BpkBaseSkeleton.module.scss

+  }
+}
+
+@keyframes shimmer-anim {


Is it better to write its name clearly here, such as shimmer-animation?

lynn-sc · 2024-04-10T08:00:55Z

packages/bpk-component-skeleton/src/BpkBaseSkeleton.tsx

+  const classNames = [getClassName('bpk-skeleton')];
+  if (className) {
+    classNames.push(className);
+  }


Could this be simplified to const classNames = getClassName('bpk-skeleton', className)? What do you think?

lynn-sc · 2024-04-10T08:03:40Z

packages/bpk-component-skeleton/src/BpkBaseSkeleton.tsx

+  }
+
+  return (
+    <div role="status" aria-label={ariaLabel} className={classNames.join(' ')} style={styleObj}  />


So I think className={classNames.join(' ')} can be className={classNames}

lynn-sc · 2024-04-10T08:06:08Z

packages/bpk-component-skeleton/src/BpkBaseSkeleton.tsx

+type Props = {
+  className: string;
+  ariaLabel?: string;
+  styleObj?: {
+    width: string;
+    height: string;
+  };
+};
+
+const BpkBaseSkeleton = ({
+  ariaLabel,
+  className,
+  styleObj
+ }: Props) => {


I see that ariaLabel and styleObj are optional. So could we assign initial values to them?

lynn-sc · 2024-04-10T09:02:23Z

packages/bpk-component-skeleton/src/BpkBodyTextSkeleton.tsx

+  ariaLabel,
+  className,


lynn-sc · 2024-04-10T09:10:38Z

packages/bpk-component-skeleton/src/BpkCircleSkeleton.tsx

+  ariaLabel,
+  className,


lynn-sc · 2024-04-10T09:10:59Z

packages/bpk-component-skeleton/src/BpkHeadlineSkeleton.tsx

+  ariaLabel,
+  className,


lynn-sc · 2024-04-10T09:11:18Z

packages/bpk-component-skeleton/src/BpkImageSkeleton.tsx

+  ariaLabel,
+  className,


github-actions · 2024-04-12T09:22:29Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-12T09:29:41Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-12T09:38:00Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-18T08:24:54Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-18T10:50:26Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-19T11:40:35Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

…github.com/Skyscanner/backpack into MAT-1205_implment_skeleton_with_fully_test

github-actions · 2024-04-19T11:56:45Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

olliecurtis · 2024-04-22T10:09:46Z

packages/bpk-component-skeleton/src/BpkSkeleton.tsx

+type ComponentProps = {
+  type: Extract<SkeletonType, 'image'>
+  size?: SizeMap['image'] | CUSTOM_SIZE_TYPE;
+  ariaLabel: string;


Just speaking with A11y folks on this one we will not want to have an ariaLabel at all on this Skeleton component and actually the skeleton should be aria-hidden until it loads.

I have updated the API in the Figma design to reflect this

I see. Thank you for confirming. Let me update the pr.

olliecurtis · 2024-04-22T10:14:49Z

packages/bpk-component-skeleton/src/BpkSkeleton.tsx

+  const classNames:string[] = [getClassName(`bpk-skeleton__${type}`)];
+  let styleObj;
+
+  if(typeof size === 'object') {
+    styleObj = size;
+  } else {
+    classNames.push(getClassName(`bpk-skeleton__${type}--${size}`))
+  }
+
+  if(type === SKELETON_TYPES.image) {
+    if(props.style === IMAGE_SKELETON_STYLE.rounded) {
+      classNames.push(getClassName('bpk-skeleton__image--rounded'));
+    }
+  }


This can be simplified to be

Suggested change

const classNames:string[] = [getClassName(`bpk-skeleton__${type}`)];

let styleObj;

if(typeof size === 'object') {

styleObj = size;

} else {

classNames.push(getClassName(`bpk-skeleton__${type}--${size}`))

}

if(type === SKELETON_TYPES.image) {

if(props.style === IMAGE_SKELETON_STYLE.rounded) {

classNames.push(getClassName('bpk-skeleton__image--rounded'));

}

}

const isImageRounded = type === SKELETON_TYPES.image && props.style === IMAGE_SKELETON_STYLE.rounded

const classNames:string = getClassName(

`bpk-skeleton__${type}`,

typeof size != 'object' && `bpk-skeleton__${type}--${size}`,

isImageRounded && 'bpk-skeleton__image--rounded'

);

const styleObj = typeof size === 'object' ? size : null;

Updated. There is a little difference, if the type of size is not object, then styleObj is set to undefined. Because to set the style attribute to an element, it must be either object or undefined.

olliecurtis · 2024-04-22T10:15:45Z

packages/bpk-component-skeleton/src/BpkSkeleton.tsx

+export const SIZE_TYPES = {
+  small: 'small',
+  default: 'default',
+  large: 'large',
+} as const;
+
+export type SizeType = (typeof SIZE_TYPES)[keyof typeof SIZE_TYPES] | CUSTOM_SIZE_TYPE;
+
+export const SKELETON_TYPES = {
+  image: 'image',
+  bodyText: 'bodyText',
+  circle: 'circle',
+  headline: 'headline',
+} as const;
+
+export type SkeletonType = (typeof SKELETON_TYPES)[keyof typeof SKELETON_TYPES];
+
+export const IMAGE_SKELETON_STYLE = {
+  rounded: 'rounded',
+  default: 'default',
+} as const;
+export type ImageSkeletonStyle = (typeof IMAGE_SKELETON_STYLE)[keyof typeof IMAGE_SKELETON_STYLE];
+
+type SizeMap = {
+  [SKELETON_TYPES.image]: (typeof SIZE_TYPES)['default'];
+  [SKELETON_TYPES.bodyText]: (typeof SIZE_TYPES)[keyof typeof SIZE_TYPES];
+  [SKELETON_TYPES.circle]: Exclude<(typeof SIZE_TYPES)[keyof typeof SIZE_TYPES], 'large'>
+  [SKELETON_TYPES.headline]: (typeof SIZE_TYPES)[keyof typeof SIZE_TYPES];
+};


Thinking of these to make this file a bit more readable to place these inside the common-types.tsx file

Yes, just like what BpkButtonV2 does. Updated.

github-actions · 2024-04-23T06:38:43Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-23T08:29:29Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

github-actions · 2024-04-23T08:38:14Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

olliecurtis · 2024-04-23T13:28:00Z

packages/bpk-component-skeleton/src/BpkSkeleton.tsx

+BpkSkeleton.defaultProps = {
+  size: SIZE_TYPES.default,
+  style: IMAGE_SKELETON_STYLE.default,
+}


In TS we wouldn't declare default props like this but more up in the props definition on L31

Would it be this?

The reason why I don't deconstruct at line L31 is that it will cause ts error. (The style property only exist when type is 'image')

github-actions · 2024-04-24T08:04:03Z

Visit https://backpack.github.io/storybook-prs/3337 to see this build running in a browser.

…3337) * implement skeleton * add declare file * [MAT-1241] add test files (#3340) * add test file * add accessibility test --------- Co-authored-by: Felix luo <felix.luo@skyscanner.net> * update examples and stories (#3341) Co-authored-by: Felix luo <felix.luo@skyscanner.net> * update readme file (#3342) Co-authored-by: Felix luo <felix.luo@skyscanner.net> * add role for base skeleton component * optimize code and remove visual test * update lint * add a combined component example * Refactored the code (#3352) Co-authored-by: Felix luo <felix.luo@skyscanner.net> * rename className to skeletonStyle * update test * update scss * update examples * use vertical style on examples * remove ariaLabel, simplify bpkskeleton and categorize types * update readme * change the declare of default value --------- Co-authored-by: Felix luo <felix.luo@skyscanner.net>

implement skeleton

c7ee3a0

add declare file

3dad2c6

felix-luo-sc added the minor Non breaking change label Apr 8, 2024

Merge branch 'main' into MAT-1205_implment_skeleton_with_fully_test

14bb615

[MAT-1241] add test files (#3340)

da71018

* add test file * add accessibility test --------- Co-authored-by: Felix luo <felix.luo@skyscanner.net>

update examples and stories (#3341)

4958df1

Co-authored-by: Felix luo <felix.luo@skyscanner.net>

update readme file (#3342)

c46dfd0

Co-authored-by: Felix luo <felix.luo@skyscanner.net>

Felix luo and others added 2 commits April 9, 2024 16:55

add role for base skeleton component

16aef33

Merge branch 'main' into MAT-1205_implment_skeleton_with_fully_test

aab748f

felix-luo-sc changed the title ~~[MAT-1205] implement skeleton~~ [MAT-1205] [BpkSkeleton] Add new component - bpk-component-skeleton Apr 9, 2024

olliecurtis reviewed Apr 9, 2024

View reviewed changes

examples/bpk-component-skeleton/stories.ts Outdated Show resolved Hide resolved

olliecurtis self-assigned this Apr 9, 2024

lynn-sc reviewed Apr 10, 2024

View reviewed changes

packages/bpk-component-skeleton/src/BpkBodyTextSkeleton.tsx Outdated

Comment on lines 45 to 46

ariaLabel,

className,

Copy link

lynn-sc Apr 10, 2024

Choose a reason for hiding this comment

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

Same here

lynn-sc reviewed Apr 10, 2024

View reviewed changes

packages/bpk-component-skeleton/src/BpkCircleSkeleton.tsx Outdated

Comment on lines 44 to 45

ariaLabel,

className,

Copy link

lynn-sc Apr 10, 2024

Choose a reason for hiding this comment

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

Same here

lynn-sc reviewed Apr 10, 2024

View reviewed changes

packages/bpk-component-skeleton/src/BpkHeadlineSkeleton.tsx Outdated

Comment on lines 45 to 46

ariaLabel,

className,

Copy link

lynn-sc Apr 10, 2024

Choose a reason for hiding this comment

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

Same above

lynn-sc reviewed Apr 10, 2024

View reviewed changes

packages/bpk-component-skeleton/src/BpkImageSkeleton.tsx Outdated

Comment on lines 50 to 51

ariaLabel,

className,

Copy link

lynn-sc Apr 10, 2024

Choose a reason for hiding this comment

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

Same above

optimize code and remove visual test

d768a4f

update test

861f5a3

update scss

14808e3

Merge branch 'main' into MAT-1205_implment_skeleton_with_fully_test

2b021d4

Merge branch 'main' into MAT-1205_implment_skeleton_with_fully_test

59c78fd

Felix luo and others added 2 commits April 19, 2024 19:36

update examples

b251c68

Merge branch 'main' into MAT-1205_implment_skeleton_with_fully_test

32ef3d4

Felix luo added 2 commits April 19, 2024 19:49

use vertical style on examples

ff3b667

Merge branch 'MAT-1205_implment_skeleton_with_fully_test' of https://…

e6a1943

…github.com/Skyscanner/backpack into MAT-1205_implment_skeleton_with_fully_test

olliecurtis requested changes Apr 22, 2024

View reviewed changes

Merge branch 'main' into MAT-1205_implment_skeleton_with_fully_test

edf5a13

remove ariaLabel, simplify bpkskeleton and categorize types

233fdbb

update readme

cf50b02

olliecurtis reviewed Apr 23, 2024

View reviewed changes

change the declare of default value

a7b8095

olliecurtis approved these changes Apr 24, 2024

View reviewed changes

olliecurtis merged commit 594f25a into main Apr 24, 2024
9 checks passed

olliecurtis deleted the MAT-1205_implment_skeleton_with_fully_test branch April 24, 2024 12:33

[MAT-1205] [BpkSkeleton] Add new component - bpk-component-skeleton #3337

[MAT-1205] [BpkSkeleton] Add new component - bpk-component-skeleton #3337

Conversation

felix-luo-sc commented Apr 7, 2024 • edited

Figma

github-actions bot commented Apr 7, 2024 • edited

Browser support

github-actions bot commented Apr 7, 2024

github-actions bot commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

github-actions bot commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 18, 2024

github-actions bot commented Apr 18, 2024

github-actions bot commented Apr 19, 2024

github-actions bot commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felix-luo-sc Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2024

felix-luo-sc commented Apr 7, 2024 •

edited

github-actions bot commented Apr 7, 2024 •

edited

felix-luo-sc Apr 23, 2024 •

edited