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

[Mobile] Update the bottom sheet header #34309

Merged
merged 9 commits into from
Sep 7, 2021
Merged

[Mobile] Update the bottom sheet header #34309

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2770d9d
Add new Header bottomSheet component
enejb Aug 25, 2021
8e07e65
Include the new Header component in buttomSheet
enejb Aug 25, 2021
8e7a218
Update NavigationHeader to use the new Header component for backwards…
enejb Aug 25, 2021
1d2df4d
Update all instances of NavigationHeader with Header
enejb Aug 25, 2021
a716349
Update BottomSheet Header documentation to reduce its size (#34339)
dcalhoun Aug 26, 2021
1f3d204
Move Header to NavBar
enejb Sep 4, 2021
ee17e3d
Update usage from Header to NavBar
enejb Sep 4, 2021
1497dd0
Remove the NavigationHeader component
enejb Sep 4, 2021
c1ac20d
Fix space in the styles
enejb Sep 4, 2021
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
10 changes: 6 additions & 4 deletions packages/components/src/font-size-picker/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ function FontSizePicker( {
showSheet={ showSubSheet }
>
<>
<BottomSheet.NavigationHeader
screen={ label }
leftButtonOnPress={ goBack }
/>
<BottomSheet.Header>
<BottomSheet.Header.BackButton onPress={ goBack } />
<BottomSheet.Header.Title>
{ label }
</BottomSheet.Header.Title>
</BottomSheet.Header>
Copy link
Member

Choose a reason for hiding this comment

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

I have largely seen "compound" components used to share state via React Context. I have not often seen "compound" components created largely for style application, i.e. providing the structural styles needed to keep the header UI consistent.

I think this use is really smart, and could work well for (1) ensuring the styles are applied consistently and (2) provide the ability to compose the elements as necessary for each context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BottomSheet.Header.Title
I am not really sure if this is desired. I kind of like it mostly because it gives you a context where the compone is meant to be used but this same "advantage" makes it harder to reuse the same component.

For example.

<BottomSheet.Footer>
<Header.Title>Some footer title</Header.Title>
</BottomSheet.Footer>

but I guess at that point we could refactor it to look be part of the bottom sheet title component. But all that could be avoided if we made the component a bottom sheet component at this point. Thought?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought, but maybe "title" is not the best choice here 🤔 imho there should only be one title on a sheet.. so maybe label or heading is a better name?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with both of your perspectives. I suppose there is a fine line between naming the is too restrictive and naming so generic that it does not communicate intent.

I do like "heading" instead of "title." If we desire to allow these components to be used in a footer, we could replace "header" with "navbar."

<BottomSheet.NavBar>
  <BottomSheet.NavBar.Heading>A Footer Component</BottomSheet.NavBar.Heading>
</BottomSheet.NavBar>

<View style={ styles[ 'components-font-size-picker' ] }>
<BottomSheet.Cell
customActionButton
Expand Down
10 changes: 6 additions & 4 deletions packages/components/src/mobile/bottom-sheet-select-control/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,12 @@ const BottomSheetSelectControl = ( {
showSheet={ showSubSheet }
>
<>
<BottomSheet.NavigationHeader
screen={ label }
leftButtonOnPress={ goBack }
/>
<BottomSheet.Header>
<BottomSheet.Header.BackButton onPress={ goBack } />
<BottomSheet.Header.Title>
{ label }
</BottomSheet.Header.Title>
</BottomSheet.Header>
<View style={ styles.selectControl }>
{ items.map( ( item, index ) => (
<BottomSheet.Cell
Expand Down
10 changes: 6 additions & 4 deletions packages/components/src/mobile/bottom-sheet-text-control/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,12 @@ const BottomSheetTextControl = ( {
showSheet={ showSubSheet }
>
<>
<BottomSheet.NavigationHeader
screen={ label }
leftButtonOnPress={ goBack }
/>
<BottomSheet.Header>
<BottomSheet.Header.BackButton onPress={ goBack } />
<BottomSheet.Header.Title>
{ label }
</BottomSheet.Header.Title>
</BottomSheet.Header>
<PanelBody style={ horizontalBorderStyle }>
<TextInput
label={ label }
Expand Down
67 changes: 67 additions & 0 deletions packages/components/src/mobile/bottom-sheet/header/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# BottomSheet Header

BottomSheet Header components provide styled elements for composing header UI within a `BottomSheet`.

## Usage

```jsx
import { BottomSheet } from '@wordpress/components';

export default = () => (
<BottomSheet>
<BottomSheet.Header>
<BottomSheet.Header.BackButton onPress={ () => {} } />
<BottomSheet.Header.Title>A Sheet Title</BottomSheet.Header.Title>
<BottomSheet.Header.ApplyButton onPress={ () => {} } />
</BottomSheet.Header>
</BottomSheet>
);
```

## BottomSheet.Header

Provides structural styles for left-center-right layout for header UI.

## BottomSheet.Header.Title

Displays a styled title for a bottom sheet.

## BottomSheet.Header.ApplyButton

Displays a styled button to apply settings of bottom sheet controls.

### Props

#### onPress

Callback invoked once the button is pressed.

## BottomSheet.Header.BackButton

Displays a styled button to navigate backwards from a bottom sheet.

### Props

#### onPress

Callback invoked once the button is pressed.

## BottomSheet.Header.CancelButton

Displays a styled button to dismiss a full screen bottom sheet.

### Props

#### onPress

Callback invoked once the button is pressed.

## BottomSheet.Header.CloseButton

Displays a styled button to dismiss a full screen bottom sheet.

### Props

#### onPress

Callback invoked once the button is pressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Though the description here is the same for both of these components, I'm wondering about their intended semantics, and whether they really are the same. I think that Cancel implies "undoing" some changes that were made, while Close does not imply that. Otoh, what actually happens is entirely dependent on the implementation in the callback. Maybe we can find a way to define the difference between these components in the readme to guide developers that use these components toward a more consistent usage?

Copy link
Member

Choose a reason for hiding this comment

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

Otoh, what actually happens is entirely dependent on the implementation in the callback.

That was my original thought with the documentation for the onPress callback. The current description is very much focused on what does occur, not the intended usage.

Maybe we can find a way to define the difference between these components in the readme to guide developers that use these components toward a more consistent usage?

Counter to the current documentation (I modified) and my aforementioned original thought, I do understand and agree there is value in describing the intent. The description of the components themselves differ somewhat from the onPress prop description. E.g. ApplyButton states "Displays a styled button to apply settings of bottom sheet controls," which describe the intent to apply the settings.

If we want to update the documentation to better describe the intent of the components and props, that is fine with me. Apologies if my previous changes removed too much of that, and thus creating more work to add the intent back. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies if my previous changes removed too much of that, and thus creating more work to add the intent back.

No worries! I think it's great to discuss this and I'm happy this is being addressed. The usage of these patterns will certainly grow, so it's valuable to consider many paths. There is a tendency (and maybe a goal?) to make changes to block settings immediately reflected in the block rendering (i.e. no "confirm" or "cancel" needed). I think this can't cover the case for any fullscreen sheets, though, so this may be at least one source of tension. Historically, though, this may have been addressed in a somewhat ad-hoc way, giving rise to some redundancy in naming — but that's just my guess.

In any case, removing truly synonymous components and clarifying the usage of the remaining ones is a great idea. Also, I especially like the idea mentioned here to add iosText as a prop. This accurate signalling provides immediate clarity, whereas the current form feels like it is violating the principle of least surprise, imo.

29 changes: 29 additions & 0 deletions packages/components/src/mobile/bottom-sheet/header/action-button.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* External dependencies
*/
import { View, TouchableWithoutFeedback } from 'react-native';

/**
* Internal dependencies
*/
import styles from './styles.scss';

function ActionButton( {
onPress,
accessibilityLabel,
accessibilityHint,
children,
} ) {
return (
<TouchableWithoutFeedback
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved
onPress={ onPress }
accessibilityRole={ 'button' }
accessibilityLabel={ accessibilityLabel }
accessibilityHint={ accessibilityHint }
>
<View style={ styles[ 'action-button' ] }>{ children }</View>
</TouchableWithoutFeedback>
);
}

export default ActionButton;
53 changes: 53 additions & 0 deletions packages/components/src/mobile/bottom-sheet/header/apply-button.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/**
* External dependencies
*/
import { View, Text, Platform } from 'react-native';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Icon, check } from '@wordpress/icons';
import { usePreferredColorSchemeStyle } from '@wordpress/compose';

/**
* Internal dependencies
*/
import styles from './styles.scss';
import ActionButton from './action-button';

function ApplyButton( { onPress } ) {
const buttonTextStyle = usePreferredColorSchemeStyle(
styles[ 'button-text' ],
styles[ 'button-text-dark' ]
);

const applyButtonStyle = usePreferredColorSchemeStyle(
styles[ 'apply-button-icon' ],
styles[ 'apply-button-icon-dark' ]
);

return (
<View style={ styles[ 'apply-button' ] }>
<ActionButton
onPress={ onPress }
accessibilityLabel={ __( 'Apply' ) }
accessibilityHint={ __( 'Applies the setting' ) }
>
{ Platform.OS === 'ios' ? (
<Text style={ buttonTextStyle } maxFontSizeMultiplier={ 2 }>
{ __( 'Apply' ) }
</Text>
) : (
<Icon
icon={ check }
size={ 24 }
style={ applyButtonStyle }
/>
) }
</ActionButton>
</View>
);
}

export default ApplyButton;
99 changes: 99 additions & 0 deletions packages/components/src/mobile/bottom-sheet/header/back-button.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/**
* External dependencies
*/
import { View, Platform, Text } from 'react-native';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Icon, arrowLeft, close } from '@wordpress/icons';
import { usePreferredColorSchemeStyle } from '@wordpress/compose';

/**
* Internal dependencies
*/
import styles from './styles.scss';
import ActionButton from './action-button';
import chevronBack from './../chevron-back';

function Button( { onPress, icon, text } ) {
const buttonTextStyle = usePreferredColorSchemeStyle(
styles[ 'button-text' ],
styles[ 'button-text-dark' ]
);

return (
<View style={ styles[ 'back-button' ] }>
<ActionButton
onPress={ onPress }
accessibilityLabel={ __( 'Go back' ) }
accessibilityHint={ __(
'Navigates to the previous content sheet'
) }
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved
>
{ icon }
{ text && (
<Text style={ buttonTextStyle } maxFontSizeMultiplier={ 2 }>
{ text }
</Text>
) }
</ActionButton>
</View>
);
}

function BackButton( { onPress } ) {
const chevronLeftStyle = usePreferredColorSchemeStyle(
styles[ 'chevron-left-icon' ],
styles[ 'chevron-left-icon-dark' ]
);
const arrowLeftStyle = usePreferredColorSchemeStyle(
styles[ 'arrow-left-icon' ],
styles[ 'arrow-right-icon-dark' ]
);

let backIcon;
let backText;

if ( Platform.OS === 'ios' ) {
backIcon = (
<Icon icon={ chevronBack } size={ 21 } style={ chevronLeftStyle } />
);
backText = __( 'Back' );
} else {
backIcon = (
<Icon icon={ arrowLeft } size={ 24 } style={ arrowLeftStyle } />
);
}

return <Button onPress={ onPress } icon={ backIcon } text={ backText } />;
}

function CancelButton( { onPress, text } ) {
const arrowLeftStyle = usePreferredColorSchemeStyle(
styles[ 'arrow-left-icon' ],
styles[ 'arrow-right-icon-dark' ]
);

let backIcon;
let backText;

if ( Platform.OS === 'ios' ) {
backText = text ? text : __( 'Cancel' );
} else {
backIcon = <Icon icon={ close } size={ 24 } style={ arrowLeftStyle } />;
}

return <Button onPress={ onPress } icon={ backIcon } text={ backText } />;
}

function CloseButton( { onPress } ) {
return <CancelButton onPress={ onPress } text={ __( 'Close' ) } />;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the small text change to "Close" merits an entire component. I'm not sure it provides value. WDYT about leveraging CancelButton and passing text as a prop when needed instead?

<CancelButton onPress={ () => {} } text={ __( 'Close' ) } />

Copy link
Contributor Author

@enejb enejb Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I am not a big fan of the text prop. Mostly because I would expect the text to be passed down with via the children like

<Button onPress={ () => {}  }>{ __( 'Close' ) } ></Button>

but in this case, because we are not just rendering all the children but only in special cases. (only on iOS) having a prop of text could work well and we can make the component a bit more generic. and remove the need for the different components.

Another way we would accomplish the same thing would be if a had a specific some sort of type and then pass in the different ones. ( 'close', 'back', 'close' ) this would also help us set different accessibility hints in the future.
I am not a fan of this approach mostly since it creates type is a bit generic term and we end up in a place where we don't really know all the different types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming back to think I think once we have more specific accessibility Hint and specific content.
I think it would make sense to have a block for each of the different "BackButtons" s.
Also, the code would read better and we could avoid things like

<BottomSheet.Header.CancelButton onPress={ close } text={ __( 'Close' ) } />

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have strong opinions about the granularity of the components in this case, but I wanted to mention that there may be cases where "Close" "Cancel" and "Back" are use somewhat inconsistently. The componentization in this PR might also be a good opportunity to clarify the intended semantics of these.

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 not a big fan of the text prop. Mostly because I would expect the text to be passed down with via the children [...]

but in this case, because we are not just rendering all the children but only in special cases. (only on iOS) having a prop of text could work well and we can make the component a bit more generic. and remove the need for the different components.

Understandable. I think relying upon children over text generally makes sense and should be the default. In reality, the fact that the text is conditionally rendered doesn't prohibit us from setting it with children. We could still pass it as children.

That said, would it be better to communicate the conditional rendering via the property name? E.g. iosText.

<BottomSheet.Header.CancelButton iosText={ __('Close') } />

Another way we would accomplish the same thing would be if a had a specific some sort of type and then pass in the different ones. ( 'close', 'back', 'close' ) this would also help us set different accessibility hints in the future.
I am not a fan of this approach mostly since it creates type is a bit generic term and we end up in a place where we don't really know all the different types.

I agree that moving to an abstraction where there is a "type" prop is likely too far an abstraction (also CancelButton, BackButton, CloseButton is sort of already the "types"). The original limitation I ran into with the Help section refactor was that I could not pass custom text for the one-off "Cancel" text needed.

We could create "types" including one for the "Cancel" scenario, but it sort of puts us right back into the original limitation I hit. What if we encounter a new context where we need "Dismiss" text? If we encounter a new context, do we want to add another "type" or pass the required text via a freeform text prop? WDYT?

Coming back to think I think once we have more specific accessibility Hint and specific content.
I think it would make sense to have a block for each of the different "BackButtons" s.

I don't have strong opinions about the granularity of the components in this case, but I wanted to mention that there may be cases where "Close" "Cancel" and "Back" are use somewhat inconsistently. The componentization in this PR might also be a good opportunity to clarify the intended semantics of these.

The last two quotes are from each of @enejb and @mkevins separately, but I think you are both saying the same thing — that you might prefer to keep CloseButton. If I am interpreting that correctly, that is fine with me. My original question of its existence is loosely held. I understand how creating it as a separate component to mange the text and any other accessibility properties provides value. 👍🏻


Button.Back = BackButton;
Button.Cancel = CancelButton;
Button.Close = CloseButton;

export default Button;
24 changes: 24 additions & 0 deletions packages/components/src/mobile/bottom-sheet/header/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* External dependencies
*/
import { View } from 'react-native';

/**
* Internal dependencies
*/
import styles from './styles.scss';
import Button from './back-button';
import ApplyButton from './apply-button';
import Title from './title';

function Header( { children } ) {
return <View style={ styles.header }>{ children }</View>;
}

Header.ApplyButton = ApplyButton;
Header.BackButton = Button.Back;
Header.CancelButton = Button.Cancel;
Header.CloseButton = Button.Close;
Header.Title = Title;

export default Header;
71 changes: 71 additions & 0 deletions packages/components/src/mobile/bottom-sheet/header/styles.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@

.header {
align-items: center;
flex-direction: row;
height: 44px;
justify-content: center;
}

.title {
color: $light-primary;
text-align: center;
font-weight: 600;
font-size: 16px;
position: absolute;
Copy link
Member

Choose a reason for hiding this comment

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

When I worked on the refactor for the Help section, I attempted to create the left-center-right layout while avoiding absolute positioning. I did this largely to reduce the likelihood of UI collisions. E.g. what happens with a really long title? Do you think there is any value in using the flex layout approach instead of absolute positioning?

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 problem that I ran into is that the title component wasn't centred anymore once I removed the empty View component that was making things centred.
I am not sure how we can make this happen without using absolute positioning. I will see if I can remove the absolute positing by using align-items: "center".

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. If I am not mistaken, my CodePen I linked in my previous comment to does have the title centered even when the right button is not present. I used that in the original Help section header. So, I do believe it is possible.

Copy link
Member

Choose a reason for hiding this comment

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

That said, it may not be worth the effort or complexity. Absolute position may work well for our use case.

width: 100%;
}

.title-dark {
color: $dark-primary;
}

.action-button {
align-items: center;
flex-direction: row;
height: 100%;
justify-content: center;
min-width: 44px;
padding-left: $grid-unit-20;
padding-right: $grid-unit-20;
}

.back-button {
align-items: flex-start;
flex: 1;
justify-content: center;
z-index: 2;
Copy link
Member

Choose a reason for hiding this comment

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

If we avoid absolute positioning, it'd also likely negate the need for any z-index declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

.apply-button {
align-items: flex-end;
flex: 1;
justify-content: center;
z-index: 2;
}


.button-text {
color: $blue-50;
font-size: 16px;
}

.button-text-dark {
color: $blue-30;
}

.chevron-left-icon {
color: $blue-50;
margin-left: -11px;
}

.chevron-left-icon-dark {
color: $blue-30;
}

.arrow-left-icon {
color: $gray-60;
}

.arrow-left-icon-dark {
color: $dark-secondary;
}
Loading