[BD-7115] Add Section Header into Backpack #2912

KathyWang0208 · 2023-07-12T02:58:14Z

Add backpack section header component

###Snapshots

Default

Default	RTL

With description

Default	RTL

With Full props

Default	RTL

Remember to include the following changes:

README.md (If you have created a new component)
Component README.md
Tests
Storybook examples created/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 · 2023-07-12T03:07:00Z

Browser support

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

Generated by 🚫 dangerJS against 1ac15d6

github-actions · 2023-07-12T03:07:12Z

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

github-actions · 2023-07-12T04:48:14Z

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

examples/bpk-component-section-header/examples.js

packages/bpk-component-section-header/src/BpkSectionHeader.module.scss

packages/bpk-component-section-header/src/BpkSectionHeader.js

github-actions · 2023-07-13T07:59:45Z

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

anambl

Only other thing if you could please remove the flow tags as we don't need those anymore now that you've changed it to TS! 😄

anambl · 2023-07-13T10:26:37Z

examples/bpk-component-section-header/examples.module.scss

+.bpk-component-section-header {
+  &--btn {
+    display: flex;
+    width: 36 * $bpk-one-pixel-rem;


You don't need this, as button has a iconOnly prop so you don't need to set the width/height yourself 😄

anambl · 2023-07-13T10:28:14Z

examples/bpk-component-section-header/examples.tsx

+  <BpkButton
+    className={cls('bpk-component-section-header--btn')}
+    CommonProps={{ blank: false }}
+  >


What is this prop used for?

if missing it, got this warning

should I remove CommonProps?

Oh that's weird.. actually BpkButton is not written in TS. Instead you can use BpkButtonV2 which is the newer version and is written in TS as the old one will go away anyways 🙂
import { BpkButtonV2 } from '../../packages/bpk-component-button';

CommonProps is not actually a prop, I guess the TS compiler gets a bit confused with importing a JS component into a TS file 🙁

BpkButtonV2 works, thank you

anambl · 2023-07-13T10:29:01Z

examples/bpk-component-section-header/examples.tsx

+
+import STYLES from './examples.module.scss';
+
+const cls = cssModules(STYLES);


could you rename this to getClassName just to align with the rest of the usages

anambl · 2023-07-13T10:31:03Z

examples/bpk-component-section-header/stories.ts

+
+export const ForMobile = MobileExample;
+
+export const VisualTestDefault = DefaultExample;


No need to have a visual test for each of these. Instead you can bundle them all into one example component that gets visually tested. We do this in most of our components for example - https://github.com/Skyscanner/backpack/blob/main/examples/bpk-component-badge/examples.js#L140

anambl · 2023-07-13T10:32:02Z

packages/bpk-component-section-header/src/BpkSectionHeader.module.scss

+  justify-content: space-between;
+  align-items: flex-start;
+
+  &--titleAndDesc {


can you rename this so that it uses kebab case for consistency? We don't use camel case

github-actions · 2023-07-13T11:52:29Z

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

anambl · 2023-07-13T11:56:35Z

examples/bpk-component-section-header/examples.tsx

+
+/* @flow strict */
+
+import { get } from 'lodash';


I don't think this is used?

anambl · 2023-07-13T12:03:24Z

packages/bpk-component-section-header/README.md

+| --------------------- |----------|----------| ------------- |
+| title              | string   | true     | -             |
+| description              | string   | false    | -             |
+| button              | node     | false    | -             |


can you also add the onDark prop here please? 😄

anambl · 2023-07-13T12:04:35Z

Can you also add the type declaration files d.ts and remove the flow tags? 😄
For the type declaration files you can use the TS compiler to autogenerate them!

giriawu · 2023-07-14T02:38:48Z

examples/bpk-component-section-header/examples.module.scss

+@import '~bpk-mixins/index.scss';
+
+.bpk-section-header-examples {
+  &--on-dark {


If this is the first level, could use &__on-dark like many other cases in backpack.

Thank you for reviews, &--on-dark is acceptable like BpkModalInner.module.scss

giriawu · 2023-07-14T02:39:08Z

examples/bpk-component-section-header/examples.module.scss

+
+  &--mixed {
+    display: flex;
+    grid-row-gap: $bpk-spacing-base;


Use bpk-spacing-base()

giriawu · 2023-07-14T02:41:30Z

examples/bpk-component-section-header/examples.tsx

+ * limitations under the License.
+ */
+
+/* @flow strict */


I think if changed into typescript, this kind of exegesis could be removed.

giriawu · 2023-07-14T02:44:27Z

packages/bpk-component-section-header/README.md

+| title       | string                     | true     | -             |
+| description | string                     | false    | -             |
+| button      | node                       | false    | -             |
+| type        | oneOf("default", "onDark") | false    | default       |


Should be SECTION_TYPES.onDark and SECTION_TYPES.default

giriawu · 2023-07-14T02:45:33Z

packages/bpk-component-section-header/src/BpkSectionHeader-test.tsx

+      <BpkSectionHeader
+        title="Section title"
+        description="Section title description"
+        button={<BpkButton onClick={() => jest.fn()}>View more</BpkButton>}


Use BpkButtonV2 here to keep consistency?

github-actions · 2023-07-14T02:52:22Z

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

github-actions · 2023-07-14T03:27:45Z

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

jasonhuang-sky · 2023-07-14T03:35:46Z

examples/bpk-component-section-header/stories.ts

+ * limitations under the License.
+ */
+
+/* @flow strict */


All of this should be deleted since you are already using TypeScript for the type checking but not Flow

jasonhuang-sky · 2023-07-14T03:43:26Z

packages/bpk-component-section-header/src/BpkSectionHeader.tsx

+    <div
+      className={getClassName(
+        'bpk-section-header',
+        onDark && 'bpk-section-header--on-dark',


Suggested change

onDark && 'bpk-section-header--on-dark',

`bpk-section-header--${SECTION_TYPES}`,

&--default { color: $bpk-text-primary-day; } &--onDark { color: $bpk-text-on-dark-day; }

This will make it look more concise

jasonhuang-sky · 2023-07-14T03:43:42Z

packages/bpk-component-section-header/src/BpkSectionHeader.tsx

+      <div
+        className={getClassName(
+          'bpk-section-header--title-description',
+          onDark && 'bpk-section-header--title-description--on-dark',


jasonhuang-sky · 2023-07-14T03:44:34Z

packages/bpk-component-section-header/src/accessibility-test.tsx

+  it('should not have programmatically-detectable accessibility issues', async () => {
+    const { container } = render(<BpkSectionHeader title="Section title" />);
+    const results = await axe(container);
+    expect(results).toHaveNoViolations();
+  });


Maybe you can also add a new one for including description?

Supremeyh · 2023-07-14T03:37:59Z

packages/bpk-component-section-header/src/BpkSectionHeader.tsx

+    <div
+      className={getClassName(
+        'bpk-section-header',
+        onDark && 'bpk-section-header--on-dark',


how about simply use 'bpk-section-header--dark'

yes, I changed the onDark value as on-dark, and use bpk-section-header--on-dark

Supremeyh · 2023-07-14T03:39:13Z

packages/bpk-component-section-header/src/BpkSectionHeader.tsx

+}: Props) => {
+  const onDark = type === SECTION_TYPES.onDark;
+  return (
+    <div


Could we support Accessibility in this component

Perhaps we could add another prop as a label for the button? I don't think that the aria-label for text is needed as it's text and not an interactive element?

button is a node, can we deal with the accessibility for it in the consumed micosite, then we don't need one more prop.

@Supremeyh any thoughts from an accessibility perspective on what could be improved? 🙂
From my perspective the text elements should be fine as they are and do not need a label as labels should be used for interactive elements. The texts gets read out by screen readers. Button like Kathy said should be handled on the microsite

Thanks @anambl and @KathyWang0208! It's great to hear that the text elements are already accessible, and interactive elements button can also support on microsite side. In this case, it's fine to me.

github-actions · 2023-07-14T07:15:08Z

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

github-actions · 2023-07-14T09:01:53Z

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

github-actions · 2023-07-14T10:09:58Z

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

Build Section Header

35e1a1b

KathyWang0208 changed the title ~~Build Section Header~~ [BD-7115] Build Section Header Jul 12, 2023

Kathy0208 added 4 commits July 12, 2023 11:20

add new component in README.md

890e6f9

add new component in readme

746dc16

add new component in readme

28c373a

fix test

35b62ee

KathyWang0208 added the major Breaking change label Jul 12, 2023

KathyWang0208 added minor Non breaking change and removed major Breaking change labels Jul 12, 2023

KathyWang0208 changed the title ~~[BD-7115] Build Section Header~~ [BD-7115] Add Section Header into Backpack Jul 12, 2023

anambl requested changes Jul 12, 2023

View reviewed changes

examples/bpk-component-section-header/examples.js Outdated Show resolved Hide resolved

packages/bpk-component-section-header/src/BpkSectionHeader.module.scss Show resolved Hide resolved

packages/bpk-component-section-header/src/BpkSectionHeader.js Outdated Show resolved Hide resolved

Adjust code based on comments

ddb37e8

anambl reviewed Jul 13, 2023

View reviewed changes

Kathy0208 added 2 commits July 13, 2023 19:39

Support onDark

05f2006

udpate test

9bc0d28

anambl reviewed Jul 13, 2023

View reviewed changes

Kathy0208 added 2 commits July 14, 2023 10:38

add .d.ts file

3a6c449

revert useless code

ec6567a

giriawu reviewed Jul 14, 2023

View reviewed changes

Adjust code based comment

92a956c

jasonhuang-sky reviewed Jul 14, 2023

View reviewed changes

Supremeyh reviewed Jul 14, 2023

View reviewed changes

improve the code

f54cfbe

remove flow tag

da127bd

remove the aria-label from BpkText

1ac15d6

anambl approved these changes Jul 17, 2023

View reviewed changes

anambl merged commit b729f69 into main Jul 17, 2023
7 checks passed

anambl deleted the BD-7115-BPK-section-header branch July 17, 2023 08:39


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

		const cls = cssModules(STYLES);


		export const ForMobile = MobileExample;

		export const VisualTestDefault = DefaultExample;

	onDark && 'bpk-section-header--on-dark',
	`bpk-section-header--${SECTION_TYPES}`,

[BD-7115] Add Section Header into Backpack #2912

[BD-7115] Add Section Header into Backpack #2912

Conversation

KathyWang0208 commented Jul 12, 2023 • edited Loading

Default

With description

With Full props

github-actions bot commented Jul 12, 2023 • edited Loading

Browser support

github-actions bot commented Jul 12, 2023

github-actions bot commented Jul 12, 2023

github-actions bot commented Jul 13, 2023

anambl left a 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 Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anambl commented Jul 13, 2023

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 Jul 14, 2023

github-actions bot commented Jul 14, 2023

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

KathyWang0208 Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KathyWang0208 Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

anambl Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

KathyWang0208 commented Jul 12, 2023 •

edited

Loading

github-actions bot commented Jul 12, 2023 •

edited

Loading

KathyWang0208 Jul 14, 2023 •

edited

Loading

KathyWang0208 Jul 14, 2023 •

edited

Loading

anambl Jul 14, 2023 •

edited

Loading