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.

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

Refactor heading block to share more code with web version. #20745

Merged
merged 15 commits into from
Mar 25, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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: 10 additions & 0 deletions packages/block-editor/src/components/colors/use-colors.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const TextColor = ( props ) => <>{ props.children }</>;

const InspectorControlsColorPanel = () => null;

export default function __experimentalUseColors() {
return {
TextColor,
InspectorControlsColorPanel,
};
}
39 changes: 21 additions & 18 deletions packages/block-library/src/heading/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import HeadingToolbar from './heading-toolbar';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { PanelBody } from '@wordpress/components';
import { PanelBody, __experimentalText as Text } from '@wordpress/components';
import { createBlock } from '@wordpress/blocks';
import {
AlignmentToolbar,
Expand All @@ -22,7 +22,7 @@ import {
__experimentalUseColors,
__experimentalBlock as Block,
} from '@wordpress/block-editor';
import { useRef } from '@wordpress/element';
import { useRef, Platform } from '@wordpress/element';

function HeadingEdit( { attributes, setAttributes, mergeBlocks, onReplace } ) {
const ref = useRef();
Expand All @@ -42,8 +42,8 @@ function HeadingEdit( { attributes, setAttributes, mergeBlocks, onReplace } ) {
<>
<BlockControls>
<HeadingToolbar
minLevel={ 2 }
maxLevel={ 5 }
minLevel={ Platform.OS === 'web' ? 2 : 1 }
maxLevel={ Platform.OS === 'web' ? 5 : 7 }
selectedLevel={ level }
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
Expand All @@ -56,20 +56,22 @@ function HeadingEdit( { attributes, setAttributes, mergeBlocks, onReplace } ) {
} }
/>
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Heading settings' ) }>
<p>{ __( 'Level' ) }</p>
<HeadingToolbar
isCollapsed={ false }
minLevel={ 1 }
maxLevel={ 7 }
selectedLevel={ level }
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
}
/>
</PanelBody>
</InspectorControls>
{ Platform.OS === 'web' && (
<InspectorControls>
<PanelBody title={ __( 'Heading settings' ) }>
<Text variant="label">{ __( 'Level' ) }</Text>
<HeadingToolbar
isCollapsed={ false }
minLevel={ 1 }
maxLevel={ 7 }
selectedLevel={ level }
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
}
/>
</PanelBody>
</InspectorControls>
) }
{ InspectorControlsColorPanel }
<TextColor>
<RichText
Expand Down Expand Up @@ -97,6 +99,7 @@ function HeadingEdit( { attributes, setAttributes, mergeBlocks, onReplace } ) {
[ `has-text-align-${ align }` ]: align,
} ) }
placeholder={ placeholder || __( 'Write heading…' ) }
textAlign={ align }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only thing that's bothering me, in this PR. It's redundunt with the style prop in the web and it can easily get removed since it's not used at all in the web version.

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 is a recurrent issue we have on the mobile implementations. We don't have access to CSS so we end up recurring to extra props or container classes to do the work that CSS does. I'm all open to suggestions on how can we avoid these issues.
@mtias told me there is some approach being discussed about Global Styling approach using props for the components that could be a way to make the implementation less dependent of CSS styling.

/>
</TextColor>
</>
Expand Down
81 changes: 0 additions & 81 deletions packages/block-library/src/heading/edit.native.js

This file was deleted.

4 changes: 2 additions & 2 deletions packages/block-library/src/heading/heading-level-icon.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ export default function HeadingLevelIcon( { level, isPressed = false } ) {

return (
<SVG
width="20"
height="20"
width="24"
height="24"
viewBox="0 0 20 20"
xmlns="http://www.w3.org/2000/svg"
isPressed={ isPressed }
Expand Down
174 changes: 171 additions & 3 deletions packages/components/src/dropdown-menu/index.native.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,173 @@
function DropdownMenu() {
return null;
/**
* External dependencies
*/
import classnames from 'classnames';
import { flatMap, isEmpty, isFunction } from 'lodash';
/**
* WordPress dependencies
*/
import { DOWN } from '@wordpress/keycodes';
import deprecated from '@wordpress/deprecated';
import { BottomSheet, PanelBody } from '@wordpress/components';
import { withPreferredColorScheme } from '@wordpress/compose';

/**
* Internal dependencies
*/
import Button from '../button';
import Dropdown from '../dropdown';

function mergeProps( defaultProps = {}, props = {} ) {
const mergedProps = {
...defaultProps,
...props,
};

if ( props.className && defaultProps.className ) {
mergedProps.className = classnames(
props.className,
defaultProps.className
);
}

return mergedProps;
}

function DropdownMenu( {
children,
className,
controls,
icon = 'menu',
label,
popoverProps,
toggleProps,
// The following props exist for backward compatibility.
menuLabel,
position,
} ) {
if ( menuLabel ) {
deprecated( '`menuLabel` prop in `DropdownComponent`', {
alternative: '`menuProps` object and its `aria-label` property',
plugin: 'Gutenberg',
} );
}

if ( position ) {
deprecated( '`position` prop in `DropdownComponent`', {
alternative: '`popoverProps` object and its `position` property',
plugin: 'Gutenberg',
} );
}

if ( isEmpty( controls ) && ! isFunction( children ) ) {
return null;
}

// Normalize controls to nested array of objects (sets of controls)
let controlSets;
if ( ! isEmpty( controls ) ) {
controlSets = controls;
if ( ! Array.isArray( controlSets[ 0 ] ) ) {
controlSets = [ controlSets ];
}
}
const mergedPopoverProps = mergeProps(
{
className: 'components-dropdown-menu__popover',
position,
},
popoverProps
);

return (
<Dropdown
className={ classnames( 'components-dropdown-menu', className ) }
popoverProps={ mergedPopoverProps }
renderToggle={ ( { isOpen, onToggle } ) => {
const openOnArrowDown = ( event ) => {
if ( ! isOpen && event.keyCode === DOWN ) {
event.preventDefault();
event.stopPropagation();
onToggle();
}
};
const mergedToggleProps = mergeProps(
{
className: classnames(
'components-dropdown-menu__toggle',
{
'is-opened': isOpen,
}
),
},
toggleProps
);

return (
<Button
{ ...mergedToggleProps }
icon={ icon }
onClick={ ( event ) => {
onToggle( event );
if ( mergedToggleProps.onClick ) {
mergedToggleProps.onClick( event );
}
} }
onKeyDown={ ( event ) => {
openOnArrowDown( event );
if ( mergedToggleProps.onKeyDown ) {
mergedToggleProps.onKeyDown( event );
}
} }
aria-haspopup="true"
aria-expanded={ isOpen }
label={ label }
showTooltip
>
{ mergedToggleProps.children }
</Button>
);
} }
renderContent={ ( { isOpen, onClose, ...props } ) => {
return (
SergioEstevao marked this conversation as resolved.
Show resolved Hide resolved
<BottomSheet
hideHeader={ true }
isVisible={ isOpen }
onClose={ onClose }
>
{ isFunction( children ) ? children( props ) : null }
<PanelBody title={ label }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the Picker component to reuse functionality? (packages/components/src/mobile/picker/)

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 Picker is using an ActionSheet in iOS that doesn't translate to the interface we want here.
Or are you saying to refactor the Picker component to be like like this visually?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought we wanted the ActionSheet on iOS based on @iamthomasbishop 's comment.

Still, as this layout is already in place somewhere else, maybe we could add a prop to the picker component, something like useNativeActionSheet, and use the code currently from the Android implementation if that prop is false. Anyway, not sure if this is the best way to go or if it will make things unnecessary complicated.

{ flatMap(
controlSets,
( controlSet, indexOfSet ) =>
controlSet.map(
( control, indexOfControl ) => (
<BottomSheet.Cell
key={ [
indexOfSet,
indexOfControl,
].join() }
label={ control.title }
onPress={ () => {
onClose();
if ( control.onClick ) {
control.onClick();
}
} }
editable={ false }
icon={ control.icon }
leftAlign={ true }
isSelected={ control.isActive }
/>
)
)
) }
</PanelBody>
</BottomSheet>
);
} }
/>
);
}

export default DropdownMenu;
export default withPreferredColorScheme( DropdownMenu );
1 change: 1 addition & 0 deletions packages/components/src/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export { default as withFocusOutside } from './higher-order/with-focus-outside';
export { default as withFocusReturn } from './higher-order/with-focus-return';
export { default as withNotices } from './higher-order/with-notices';
export { default as withSpokenMessages } from './higher-order/with-spoken-messages';
export * from './text';
SergioEstevao marked this conversation as resolved.
Show resolved Hide resolved

// Mobile Components
export { default as BottomSheet } from './mobile/bottom-sheet';
Expand Down
Loading