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

Jump to bottom

[IRN-5018][BpkNavigationBar][BpkBottomSheet] Add handling for long title text #3393

Merged
merged 10 commits into from
May 2, 2024
Merged

[IRN-5018][BpkNavigationBar][BpkBottomSheet] Add handling for long title text #3393

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
31ddb69
Add example heading which doesn't wrap properly
steviehailey-skyscanner Apr 29, 2024
dd898db
Add example of issue as navigation bar story
steviehailey-skyscanner Apr 29, 2024
428256c
Adjust navbar and bottom sheet based on design
steviehailey-skyscanner Apr 29, 2024
17b72f6
Fix alignment for graphic header
steviehailey-skyscanner Apr 29, 2024
bd2ebf1
Apply correct sizings and margins
steviehailey-skyscanner Apr 29, 2024
b289a27
Update snaps
steviehailey-skyscanner Apr 30, 2024
600844f
Move additional padding to BottomSheet only
steviehailey-skyscanner May 1, 2024
23fb2b9
Generate missing typedefs
steviehailey-skyscanner May 1, 2024
945a874
PR feedback - prop defaults
steviehailey-skyscanner May 1, 2024
7945dd8
PR feedback - prop default
steviehailey-skyscanner May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 24 additions & 2 deletions examples/bpk-component-bottom-sheet/examples.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,26 @@ const OverflowingExample = () => (
</BottomSheetContainer>
);

const LongHeaderTextExample = () => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Demo of misbehaving header at Bottom Sheet level - now fixed

<BottomSheetContainer
title="Bottom Sheet title which is long"
closeLabel="Close Bottom Sheet"
>
This is a default bottom sheet. You can put anything you want in here.
</BottomSheetContainer>
);

const LongHeaderTextWithActionButtonExample = () => (
<BottomSheetContainer
title="Bottom Sheet title which is long"
closeLabel="Close Bottom Sheet"
actionText="Action"
onAction={action('Action clicked')}
>
This is a default bottom sheet. You can put anything you want in here.
</BottomSheetContainer>
);

const NoHeaderExample = () => (
<BottomSheetContainer
closeLabel="Close Bottom Sheet"
Expand Down Expand Up @@ -219,8 +239,8 @@ const NestedExample = () => (
wide
closeOnEscPressed
closeOnScrimClick
>
Outer Bottom Sheet
>
Outer Bottom Sheet
<BottomSheetContainer
title="Inner Bottom Sheet"
closeLabel="Close Inner Bottom Sheet"
Expand Down Expand Up @@ -274,6 +294,8 @@ export {
BackdropClickCloseExample,
EscapeCloseExample,
OverflowingExample,
LongHeaderTextExample,
LongHeaderTextWithActionButtonExample,
NoHeaderExample,
NoHeaderWithActionButtonExample,
ActionButtonExample,
Expand Down
8 changes: 7 additions & 1 deletion examples/bpk-component-bottom-sheet/stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ import {
BackdropClickCloseExample,
EscapeCloseExample,
OverflowingExample,
LongHeaderTextExample,
LongHeaderTextWithActionButtonExample,
NoHeaderExample,
NoHeaderWithActionButtonExample,
ActionButtonExample,
WideExample,
NestedExample,
MultipleBottomSheetsExample
MultipleBottomSheetsExample,
} from './examples';

export default {
Expand All @@ -41,6 +43,10 @@ export const BackdropClickClose = BackdropClickCloseExample;
export const EscapeClose = EscapeCloseExample;
export const Overflowing = OverflowingExample;

export const LongHeaderText = LongHeaderTextExample;
export const LongHeaderTextWithActionButton =
LongHeaderTextWithActionButtonExample;

export const NoHeader = NoHeaderExample;
export const NoHeaderWithActionButton = NoHeaderWithActionButtonExample;

Expand Down
28 changes: 26 additions & 2 deletions examples/bpk-component-navigation-bar/examples.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,29 @@ const DefaultExample = () => (
</div>
);

const LongTitleTextExample = () => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Demo of misbehaving header at Nav Bar level - now fixed and inherited by Bottom Sheet

<div className={getClassNames('bpk-navigation-bar-story')}>
<BpkNavigationBar
id="test"
title="Backpack navigation bar long title example"
leadingButton={
<BpkNavigationBarIconButton
onClick={action('back clicked')}
icon={ArrowIconWithRtl}
label="back"
/>
}
trailingButton={
<BpkNavigationBarIconButton
onClick={action('close clicked')}
icon={CloseIcon}
label="close"
/>
}
/>
</div>
);

const OnDarkExample = () => (
<div className={getClassNames('bpk-navigation-bar-story')}>
<BpkNavigationBar
Expand Down Expand Up @@ -241,16 +264,17 @@ const VisualTestExample = () => (
<OnDarkExample />
<WithLinksOnDarkExample />
</div>
)
);

export {
DefaultExample,
LongTitleTextExample,
OnDarkExample,
LeadingIconOnlyExample,
TrailingIconOnlyExample,
WithLinksExample,
WithLogoExample,
WithLinksOnDarkExample,
StickyExample,
VisualTestExample
VisualTestExample,
};
5 changes: 3 additions & 2 deletions examples/bpk-component-navigation-bar/stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@
* limitations under the License.
*/


import BpkNavigationBar from '../../packages/bpk-component-navigation-bar/src/BpkNavigationBar';
import BpkNavigationBarButtonLink from '../../packages/bpk-component-navigation-bar/src/BpkNavigationBarButtonLink';
import BpkNavigationBarIconButton from '../../packages/bpk-component-navigation-bar/src/BpkNavigationBarIconButton';

import {
DefaultExample,
LongTitleTextExample,
LeadingIconOnlyExample,
TrailingIconOnlyExample,
WithLinksExample,
Expand All @@ -43,6 +43,7 @@ export default {
};

export const Default = DefaultExample;
export const LongTitleText = LongTitleTextExample;
export const OnDark = OnDarkExample;
export const LeadingIconOnly = LeadingIconOnlyExample;

Expand All @@ -57,5 +58,5 @@ export const Sticky = StickyExample;
export const VisualTest = VisualTestExample;
export const VisualTestWithZoom = VisualTest.bind({});
VisualTestWithZoom.args = {
zoomEnabled: true
zoomEnabled: true,
};
9 changes: 6 additions & 3 deletions packages/bpk-component-bottom-sheet/src/BpkBottomSheet.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
animation-timing-function: ease-in-out;
}

$close-button-width: tokens.bpk-spacing-lg() * 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unused so removed


.bpk-bottom-sheet {
z-index: tokens.$bpk-zindex-modal;
width: 100%;
Expand Down Expand Up @@ -114,8 +112,13 @@ $close-button-width: tokens.bpk-spacing-lg() * 2;
position: sticky;
top: 0;
z-index: tokens.$bpk-zindex-tooltip - 1;
padding: tokens.bpk-spacing-sm() 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding additional padding to the heading based on BottomSheet design (avoiding adding to Nav Bar directly to avoid all other consumers gaining height)


@include borders.bpk-border-bottom-sm(tokens.$bpk-line-day);
@include utils.bpk-themeable-property(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoid seeing scrolling content "through" the padding around the header
image

background-color,
--bpk-navigation-bar-background-color,
tokens.$bpk-surface-default-day
);
}

@keyframes slide-up {
Expand Down
110 changes: 55 additions & 55 deletions packages/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import BpkCloseButton from '../../bpk-component-close-button';
// @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`.
import { BpkButtonLink } from '../../bpk-component-link';
import BpkNavigationBar from "../../bpk-component-navigation-bar";
import BpkText, { TEXT_STYLES } from "../../bpk-component-text/src/BpkText";
import { TEXT_STYLES } from "../../bpk-component-text/src/BpkText";
import { BpkDialogWrapper, cssModules } from "../../bpk-react-utils";

import STYLES from './BpkBottomSheet.module.scss';
Expand Down Expand Up @@ -87,63 +87,63 @@ const BpkBottomSheet = ({
const dialogClassName = getClassName(
'bpk-bottom-sheet',
wide && 'bpk-bottom-sheet--wide'
);
);

return <BpkBreakpoint query={BREAKPOINTS.ABOVE_MOBILE}>
{(isAboveMobile: boolean) =>
return <BpkBreakpoint query={BREAKPOINTS.ABOVE_MOBILE}>
{(isAboveMobile: boolean) =>
<BpkDialogWrapper
ariaLabelledby={ariaLabelledby}
dialogClassName={dialogClassName}
id={id}
isOpen={isOpen}
onClose={(
arg0?: TouchEvent | MouseEvent | KeyboardEvent | SyntheticEvent<HTMLDialogElement, Event>,
arg1?: {
source: 'ESCAPE' | 'DOCUMENT_CLICK';
}) => handleClose( isAboveMobile ? 0 : animationTimeout, arg0, arg1)}
exiting={exiting}
transitionClassNames={{
appear: getClassName('bpk-bottom-sheet--appear'),
appearActive: getClassName('bpk-bottom-sheet--appear-active'),
exit: getClassName('bpk-bottom-sheet--exit')
}}
closeOnEscPressed={closeOnEscPressed}
closeOnScrimClick={closeOnScrimClick}
timeout={{appear: animationTimeout, exit: isAboveMobile ? 0 : animationTimeout}}
ariaLabelledby={ariaLabelledby}
dialogClassName={dialogClassName}
id={id}
isOpen={isOpen}
onClose={(
arg0?: TouchEvent | MouseEvent | KeyboardEvent | SyntheticEvent<HTMLDialogElement, Event>,
arg1?: {
source: 'ESCAPE' | 'DOCUMENT_CLICK';
}) => handleClose(isAboveMobile ? 0 : animationTimeout, arg0, arg1)}
exiting={exiting}
transitionClassNames={{
appear: getClassName('bpk-bottom-sheet--appear'),
appearActive: getClassName('bpk-bottom-sheet--appear-active'),
exit: getClassName('bpk-bottom-sheet--exit')
}}
closeOnEscPressed={closeOnEscPressed}
closeOnScrimClick={closeOnScrimClick}
timeout={{ appear: animationTimeout, exit: isAboveMobile ? 0 : animationTimeout }}
>
<>
<header className={getClassName('bpk-bottom-sheet--header')}>
<BpkNavigationBar
id={headingId}
title={title &&
<BpkText id={headingId} textStyle={TEXT_STYLES.label1} tagName="h2">{title}</BpkText>
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 can now just pass the text and options for which element and style we want (which lets the component perform the text overflow for us)

}
leadingButton={
<BpkCloseButton
label={closeLabel}
onClick={(
arg0?: TouchEvent | MouseEvent | KeyboardEvent | SyntheticEvent<HTMLDialogElement, Event>,
arg1?: {
source: 'ESCAPE' | 'DOCUMENT_CLICK';
}) => handleClose( isAboveMobile ? 0 : animationTimeout, arg0, arg1)}
/>
}
trailingButton={
actionText && onAction ? (
<BpkButtonLink
onClick={onAction}
>
{actionText}
</BpkButtonLink>
) :
null
}
/>
</header>
<div className={getClassName('bpk-bottom-sheet--content')}>{children}</div>
</>
</BpkDialogWrapper>
}
<>
<header className={getClassName('bpk-bottom-sheet--header')}>
<BpkNavigationBar
id={headingId}
title={title}
titleTextStyle={TEXT_STYLES.label1}
titleTagName={title ? "h2" : undefined}
Copy link
Member

Choose a reason for hiding this comment

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

Should we be passing in undefined or a better alternative value? Otherwise we could just do

Suggested change
titleTagName={title ? "h2" : undefined}
titleTagName={title && "h2"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the title text can be an empty string, in which case we don't really want to output an empty h2 in case causes screen-reader confusion when navigating by heading. So, undefined here is equivalent to not providing a customised tagName if we don't have a title (maintaining previous behaviour of an empty span being generated instead). If we && we'll be asking for an "" element in that scenario which TS will object to.

Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused with the response, so apologies.

Looking at the code for the NavBar if the title is empty then it wouldn't even render anything other than a div so there would always be something rendered anyway.

I am just thinking here it would be better if we passed something down even if a span to avoid using undefined as even with undefined BpkText will have a fallback anyway but it just might make sense to have somehting here so that we don't have to drill into BpkText to find out what happens if there is undefined passed down to it

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/Skyscanner/backpack/pull/3393/files#diff-8307bb32853b57180e184be5c1921bca36b55cb56431c7012848f328736f674cR89-R103

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, happy to be explicit about a span be the appropriate fallback here 👍

leadingButton={
<BpkCloseButton
label={closeLabel}
onClick={(
arg0?: TouchEvent | MouseEvent | KeyboardEvent | SyntheticEvent<HTMLDialogElement, Event>,
arg1?: {
source: 'ESCAPE' | 'DOCUMENT_CLICK';
}) => handleClose(isAboveMobile ? 0 : animationTimeout, arg0, arg1)}
/>
}
trailingButton={
actionText && onAction ? (
<BpkButtonLink
onClick={onAction}
>
{actionText}
</BpkButtonLink>
) :
null
}
/>
</header>
<div className={getClassName('bpk-bottom-sheet--content')}>{children}</div>
</>
</BpkDialogWrapper>
}
</BpkBreakpoint>
}

Expand Down
6 changes: 3 additions & 3 deletions packages/bpk-component-bottom-sheet/src/__snapshots__/BpkBottomSheet-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ exports[`BpkBottomSheet renders correctly with action props 1`] = `
class="bpk-navigation-bar__title bpk-navigation-bar__title--default"
>
<span
class="bpk-text bpk-text--heading-5"
class="bpk-text bpk-text--label-1"
Copy link
Member

Choose a reason for hiding this comment

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

Interesting these have now changed as this could lead me to believe these used to be wrong!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although since the span was empty it wouldn't have been a noticeable difference either way.

id="bpk-bottom-sheet-heading-my-bottom-sheet-bpk-navigation-bar-title"
/>
</span>
Expand Down Expand Up @@ -131,7 +131,7 @@ exports[`BpkBottomSheet renders correctly with minimum prop 1`] = `
class="bpk-navigation-bar__title bpk-navigation-bar__title--default"
>
<span
class="bpk-text bpk-text--heading-5"
class="bpk-text bpk-text--label-1"
id="bpk-bottom-sheet-heading-my-bottom-sheet-bpk-navigation-bar-title"
/>
</span>
Expand Down Expand Up @@ -200,7 +200,7 @@ exports[`BpkBottomSheet renders correctly with wide prop 1`] = `
class="bpk-navigation-bar__title bpk-navigation-bar__title--default"
>
<span
class="bpk-text bpk-text--heading-5"
class="bpk-text bpk-text--label-1"
id="bpk-bottom-sheet-heading-my-bottom-sheet-bpk-navigation-bar-title"
/>
</span>
Expand Down
14 changes: 9 additions & 5 deletions packages/bpk-component-modal/src/__snapshots__/BpkModal-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,16 @@ exports[`BpkModal should render correctly in the given target if renderTarget is
aria-labelledby="bpk-modal-heading-my-modal"
class="bpk-navigation-bar bpk-navigation-bar--default bpk-modal__navigation"
>
<h2
class="bpk-modal__heading"
id="bpk-modal-heading-my-modal"
<div
class="bpk-navigation-bar__title-container"
>
Modal title
</h2>
<h2
class="bpk-modal__heading"
id="bpk-modal-heading-my-modal"
>
Modal title
</h2>
</div>
<div
class="bpk-navigation-bar__trailing-item bpk-navigation-bar__trailing-item-default"
>
Expand Down