[IRN-5018][BpkNavigationBar][BpkBottomSheet] Add handling for long title text #3393
Conversation
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
| @@ -178,6 +178,26 @@ const OverflowingExample = () => ( | |||
| </BottomSheetContainer> | |||
| ); | |||
|
|
|||
| const WrapHeaderTextExample = () => ( | |||
There was a problem hiding this comment.
Demo the issue
| </BottomSheetContainer> | ||
| ); | ||
|
|
||
| const WrapHeaderTextWithActionButtonExample = () => ( |
There was a problem hiding this comment.
Demo issue with Action button
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
2 similar comments
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
| @@ -178,6 +178,26 @@ const OverflowingExample = () => ( | |||
| </BottomSheetContainer> | |||
| ); | |||
|
|
|||
| const LongHeaderTextExample = () => ( | |||
There was a problem hiding this comment.
Demo of misbehaving header at Bottom Sheet level - now fixed
| @@ -59,6 +59,29 @@ const DefaultExample = () => ( | |||
| </div> | |||
| ); | |||
|
|
|||
| const LongTitleTextExample = () => ( | |||
There was a problem hiding this comment.
Demo of misbehaving header at Nav Bar level - now fixed and inherited by Bottom Sheet
| @@ -30,8 +30,6 @@ | |||
| animation-timing-function: ease-in-out; | |||
| } | |||
|
|
|||
| $close-button-width: tokens.bpk-spacing-lg() * 2; | |||
There was a problem hiding this comment.
This was unused so removed
| <BpkNavigationBar | ||
| id={headingId} | ||
| title={title && | ||
| <BpkText id={headingId} textStyle={TEXT_STYLES.label1} tagName="h2">{title}</BpkText> |
There was a problem hiding this comment.
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)
| @@ -23,12 +23,19 @@ | |||
|
|
|||
| .bpk-navigation-bar { | |||
| position: relative; | |||
| display: flex; | |||
| display: grid; | |||
There was a problem hiding this comment.
Attempted to layout with flex after removing some absolute positioning on the buttons but the desired spacing and handing of buttons being optional for different scenarios goes beyond what we can easily do with Flexbox. Grid can easily achieve the different cases we need.
| padding: tokens.bpk-spacing-base(); | ||
| flex-direction: row; | ||
| justify-content: center; | ||
| padding: tokens.bpk-spacing-lg(); |
There was a problem hiding this comment.
Margin adjustments based on new design requirements
| @@ -41,40 +48,45 @@ | |||
| } | |||
|
|
|||
| &__title { | |||
| grid-column: 2; | |||
| text-align: center; | |||
| text-overflow: ellipsis; | |||
There was a problem hiding this comment.
Clamp the text for small screen/long title scenarios
| @include utils.bpk-themeable-property( | ||
| color, | ||
| --bpk-navigation-bar-title-color, | ||
| tokens.$bpk-text-primary-day | ||
| ); | ||
|
|
||
| // ensure we still apply ellipsis overflow when a custom title element is specified e.g. h2 |
There was a problem hiding this comment.
BottomSheet is an example where we want an h2 element but it needs to be treated as inline to clamp the text
| &__leading-item, | ||
| &__trailing-item { | ||
| position: absolute; |
There was a problem hiding this comment.
This positioning was causing overlapping text and buttons
| @@ -34,6 +36,8 @@ export type BarStyle = (typeof BAR_STYLES)[keyof typeof BAR_STYLES]; | |||
| export type Props = { | |||
| id: string; | |||
| title: ReactNode; | |||
| titleTextStyle?: TextStyle; | |||
There was a problem hiding this comment.
Make it easy for consumers to control the markup without needing to do their own title layout
| > | ||
| {title} | ||
| </BpkText> | ||
| </span> | ||
| ) : ( | ||
| title | ||
| <div className={getClassNames('bpk-navigation-bar__title-container')}>{title}</div> |
There was a problem hiding this comment.
Wrapper to let us layout non-text elements for consumers e.g. graphic logo scenario
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
1 similar comment
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
Fix clamp for BottomSheet h2
Make story names consistent
steviehailey-skyscanner
force-pushed
the
IRN-5018
branch
from
e95dac2
to
600844f
Compare
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
| @@ -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; | |||
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
Avoid seeing scrolling content "through" the padding around the header
| id={headingId} | ||
| title={title} | ||
| titleTextStyle={TEXT_STYLES.label1} | ||
| titleTagName={title ? "h2" : undefined} |
There was a problem hiding this comment.
Should we be passing in undefined or a better alternative value? Otherwise we could just do
| titleTagName={title ? "h2" : undefined} | |
| titleTagName={title && "h2"} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
There was a problem hiding this comment.
Sure, happy to be explicit about a span be the appropriate fallback here 👍
| @@ -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" | |||
There was a problem hiding this comment.
Interesting these have now changed as this could lead me to believe these used to be wrong!
There was a problem hiding this comment.
Indeed!
There was a problem hiding this comment.
Although since the span was empty it wouldn't have been a noticeable difference either way.
There was a problem hiding this comment.
As these have changed have we checked this is still ok on the BpkModal?
There was a problem hiding this comment.
Yeah, the behaviour in terms of not clamping the text is consistent with before but it now respects the newer margins (and avoid overlapping the buttons) so long text will be slightly different than before
New (example from: https://backpack.github.io/storybook-prs/3393/?path=/story/bpk-component-modal--long-title)
Old (existing behaviour)
There was a problem hiding this comment.
Note that this doesn't apply to the new Modal v2 which has it's own bespoke header
There was a problem hiding this comment.
We might wanna check with design if we are ok with the modal doing this in the case of these changes
Taken from (accessory-modal)[https://backpack.github.io/storybook-prs/3393/?path=/story/bpk-component-modal--with-accessory-view]
There was a problem hiding this comment.
Spoke to design and they think this scenario is unrealistic and the handling is reasonable (certainly better than the existing behaviour) so no need to adjust any further
| titleTagName, | ||
| titleTextStyle, |
There was a problem hiding this comment.
As you have made these optional, we will need to set these up as defaults otherwise it is going to cause issues when not provided.
That way you don't need to have what you have below and can just pass the props through.
Currently though there isn't even a fallback for titleTagName so that might cause some havoc
I would say here it probably should be
| titleTagName, | |
| titleTextStyle, | |
| titleTagName = 'span', | |
| titleTextStyle = TEXT_STYLES.heading5, |
steviehailey-skyscanner
changed the title
| @@ -0,0 +1,35 @@ | |||
| /* | |||
There was a problem hiding this comment.
Auto-generated missing typedefs...
| @@ -0,0 +1,41 @@ | |||
| /* | |||
There was a problem hiding this comment.
Auto-generated missing typedefs...
| @@ -0,0 +1,39 @@ | |||
| /* | |||
There was a problem hiding this comment.
Auto-generated missing typedefs...
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3393 to see this build running in a browser. |
It was discovered that long titles overlap the buttons on the BpkBottomSheet component. The issue arises from the underlying BpkNavigationBar component lacking handling for this scenario.
Fix improves layout based on design requirement to clamp the text with ellipsis when there is not enough space to fit without encroaching on buttons or wrapping to an additional line.
Screenshots
Issue of long heading overlapping buttons
Design guidance on expected layout
Examples after fix
Other selected cases - Small Mobile
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.md.d.ts) files updated