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 4 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
91 changes: 91 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,91 @@
# Header
enejb marked this conversation as resolved.
Show resolved Hide resolved

Header components is meant to be used to compose the header inside the BottomSheet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Header components is meant to be used to compose the header inside the BottomSheet.
`BottomSheet.Header` should be used to add a header to the top of a `BottomSheet` component. It makes several other components available, which can then be used to compose the header's content.

It took me a bit to fully process how this component works, so think adding a bit more detail to the intro might help with clarity. WDYT?


### Usage
See
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See

It looks like this can either be removed or perhaps there was a missing referencing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @SiobhyB The documentation section got converted updated in #34339.

```jsx
/**
* External dependencies
*/
import { View, Text } from 'react-native';
import { useNavigation } from '@react-navigation/native';

/**
* WordPress dependencies
*/
import { BottomSheet } from '@wordpress/components';

const ExampleControl = () => {

const goBack = () => {};
const applySettings = () => {};

return (
<BottomSheet.SubSheet>
<>
<BottomSheet.Header>
<BottomSheet.Header.BackButton onPress={ goBack } />
<BottomSheet.Header.Title>{ 'Howdy' }</BottomSheet.Header.Title>
<BottomSheet.Header.ApplyButton onPress={ applySetting } />
</BottomSheet.Header>
<View paddingHorizontal={ 16 }>
<Text>{ 'World' }</Text>
</View>
</>
</BottomSheet.SubSheet>
);
};

export default ExampleControl;
```

### Props
Header component doesn't have any props.

### Other Components
Other components that Header component makes available.

#### ApplyButton
```
<Header.ApplyButton onPress={ goBack } />
```

The apply button is used to apply settings of the bottom sheet control.

##### onPress
use to pass a call back once the ApplyButton is clicked.
enejb marked this conversation as resolved.
Show resolved Hide resolved

#### BackButton
```
<Header.BackButton onPress={ goBack } />
```

The back button is used to send the user back to the previous sheet.

##### onPress
use to pass a call back once the BackButton is clicked.
enejb marked this conversation as resolved.
Show resolved Hide resolved

#### CancelButton
```
<Header.CancelButton onPress={ goBack } />
```

The cancel button is used to send the user back to the previous sheet. Closes the sheet. Use this if you are using a full screen bottom sheet.

##### onPress
use to pass a call back once the CancelButton is clicked.
enejb marked this conversation as resolved.
Show resolved Hide resolved

#### CloseButton
```
<Header.CloseButton onPress={ goBack } />
```

The close button is used to closes the bottom sheet. Use this if you are using a full screen bottom sheet.

#### Title
```
<Header.Title>{ 'Howdy' }</Header.Title>
```

The title is used to display the title of the bottom sheet control.
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;
Loading