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

[RNMobile] Buttons block: Fix content justification attribute #37887

Merged
merged 7 commits into from
Jan 14, 2022
Merged

[RNMobile] Buttons block: Fix content justification attribute #37887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b7b94a
Enable layout hooks supported by native
fluiddot Jan 11, 2022
05d2688
Fix content justification in buttons block
fluiddot Jan 11, 2022
98eb9b4
Use default block layout when no layout attribute
fluiddot Jan 11, 2022
1fc638e
Use snapshot testing in border radius test case
fluiddot Jan 11, 2022
0bc7111
Create buttons block snapshots
fluiddot Jan 11, 2022
0219e5b
Add justify content test cases to buttons block
fluiddot Jan 11, 2022
1131588
Update react-native-editor changelog
fluiddot Jan 11, 2022
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
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import './generated-class-name';
import './style';
import './color';
import './font-size';
import './layout';

export { getBorderClassesAndStyles, useBorderProps } from './use-border-props';
export { getColorClassesAndStyles, useColorProps } from './use-color-props';
Expand Down
23 changes: 23 additions & 0 deletions packages/block-editor/src/hooks/layout.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* WordPress dependencies
*/
import { removeFilter } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import './layout.js';

// This filter is removed because layout styles shouldn't be added
// until layout types are supported in the native version.
removeFilter(
'editor.BlockListBlock',
'core/editor/layout/with-layout-styles'
);
Comment on lines +11 to +16
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you can see the logic related to this hook:

gutenberg/packages/block-editor/src/hooks/layout.js

Lines 183 to 226 in 29ec9e0

/**
* Override the default block element to add the layout styles.
*
* @param {Function} BlockListBlock Original component.
*
* @return {Function} Wrapped component.
*/
export const withLayoutStyles = createHigherOrderComponent(
( BlockListBlock ) => ( props ) => {
const { name, attributes } = props;
const shouldRenderLayoutStyles = hasBlockSupport(
name,
layoutBlockSupportKey
);
const id = useInstanceId( BlockListBlock );
const defaultThemeLayout = useSetting( 'layout' ) || {};
const element = useContext( BlockList.__unstableElementContext );
const { layout } = attributes;
const { default: defaultBlockLayout } =
getBlockSupport( name, layoutBlockSupportKey ) || {};
const usedLayout = layout?.inherit
? defaultThemeLayout
: layout || defaultBlockLayout || {};
const className = classnames( props?.className, {
[ `wp-container-${ id }` ]: shouldRenderLayoutStyles,
} );
return (
<>
{ shouldRenderLayoutStyles &&
element &&
createPortal(
<LayoutStyle
selector={ `.wp-container-${ id }` }
layout={ usedLayout }
style={ attributes?.style }
/>,
element
) }
<BlockListBlock { ...props } className={ className } />
</>
);
}
);


// This filter is removed because the layout controls shouldn't be
// enabled until layout types are supported in the native version.
removeFilter(
'editor.BlockEdit',
'core/editor/layout/with-inspector-controls'
);
Comment on lines +18 to +23
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you can see the logic related to this hook:

gutenberg/packages/block-editor/src/hooks/layout.js

Lines 160 to 181 in 29ec9e0

/**
* Override the default edit UI to include layout controls
*
* @param {Function} BlockEdit Original component.
*
* @return {Function} Wrapped component.
*/
export const withInspectorControls = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
const { name: blockName } = props;
const supportLayout = hasBlockSupport(
blockName,
layoutBlockSupportKey
);
return [
supportLayout && <LayoutPanel key="layout" { ...props } />,
<BlockEdit key="edit" { ...props } />,
];
},
'withInspectorControls'
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the Buttons block, regarding the justification content controls, instead of using the one provided by the layout logic:

gutenberg/packages/block-editor/src/layouts/flex.js

Lines 69 to 86 in 29ec9e0

toolBarControls: function FlexLayoutToolbarControls( {
layout = {},
onChange,
layoutBlockSupport,
} ) {
if ( layoutBlockSupport?.allowSwitching ) {
return null;
}
return (
<BlockControls group="block" __experimentalShareWithChildBlocks>
<FlexLayoutJustifyContentControl
layout={ layout }
onChange={ onChange }
isToolbar
/>
</BlockControls>
);
},

We define it within the block render:

gutenberg/packages/block-library/src/buttons/edit.native.js

Lines 126 to 138 in 29ec9e0

<BlockControls group="block">
<JustifyContentControl
allowedControls={ justifyControls }
value={ contentJustification }
onChange={ ( value ) =>
setAttributes( { contentJustification: value } )
}
popoverProps={ {
position: 'bottom right',
isAlternate: true,
} }
/>
</BlockControls>

Copy link
Contributor

Choose a reason for hiding this comment

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

Not suggesting a change here, but I'm wondering whether there is an opportunity to extract some of what is in the web implementation. Certainly not a blocker for this PR, just thinking about a possibility where FlexLayoutJustifyContentControl was extracted, if that would highlight where the platforms deviate. It may be that it isn't worth trying to merge components at that level, if it is mostly UI, but I'm thinking more for the sake of the logical parts of it, like the allowedControls, etc. 🤔

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 though the FlexLayoutJustifyContentControl component looks very coupled to the flex layout, which we don't have implemented support in the native version. In any case, I see that the only difference between the implementation of this control between Buttons block and flex layout is the support of space-between option. Maybe if we add support for that option in the native version, we might consider enabling the layout inspector tools hook:

gutenberg/packages/block-editor/src/hooks/layout.js

Lines 238 to 242 in 43f9ad7

addFilter(
'editor.BlockEdit',
'core/editor/layout/with-inspector-controls',
withInspectorControls
);

22 changes: 17 additions & 5 deletions packages/block-library/src/buttons/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
JustifyContentControl,
store as blockEditorStore,
} from '@wordpress/block-editor';
import { createBlock } from '@wordpress/blocks';
import { createBlock, getBlockSupport } from '@wordpress/blocks';
import { useResizeObserver } from '@wordpress/compose';
import { useDispatch, useSelect } from '@wordpress/data';
import { useState, useEffect, useRef, useCallback } from '@wordpress/element';
Expand All @@ -30,16 +30,23 @@ const ALLOWED_BLOCKS = [ buttonBlockName ];
const layoutProp = { type: 'default', alignments: [] };

export default function ButtonsEdit( {
attributes: { contentJustification, align },
attributes: { layout, align },
clientId,
isSelected,
setAttributes,
blockWidth,
name,
} ) {
const [ resizeObserver, sizes ] = useResizeObserver();
const [ maxWidth, setMaxWidth ] = useState( 0 );
const { marginLeft: spacing } = styles.spacing;

// Extract attributes from block layout
const layoutBlockSupport = getBlockSupport( name, '__experimentalLayout' );
const defaultBlockLayout = layoutBlockSupport?.default;
const usedLayout = layout || defaultBlockLayout || {};
Comment on lines +45 to +47
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 based on how the layout panel fetches the layout object to be used in the inspector controls (reference).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for providing all of these explanations! 😃

const { justifyContent } = usedLayout;

const { isInnerButtonSelected, shouldDelete } = useSelect(
( select ) => {
const {
Expand Down Expand Up @@ -126,9 +133,14 @@ export default function ButtonsEdit( {
<BlockControls group="block">
<JustifyContentControl
allowedControls={ justifyControls }
value={ contentJustification }
value={ justifyContent }
onChange={ ( value ) =>
setAttributes( { contentJustification: value } )
setAttributes( {
layout: {
...usedLayout,
justifyContent: value,
},
} )
}
popoverProps={ {
position: 'bottom right',
Expand All @@ -154,7 +166,7 @@ export default function ButtonsEdit( {
shouldRenderFooterAppender && renderFooterAppender.current
}
orientation="horizontal"
horizontalAlignment={ contentJustification }
horizontalAlignment={ justifyContent }
onDeleteBlock={ shouldDelete ? remove : undefined }
onAddBlock={ onAddNextButton }
parentWidth={ maxWidth } // This value controls the width of that the buttons are able to expand to.
Expand Down
27 changes: 27 additions & 0 deletions packages/block-library/src/buttons/test/__snapshots__/edit.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Buttons block justify content sets Justify items center option 1`] = `
"<!-- wp:buttons {\\"layout\\":{\\"type\\":\\"flex\\",\\"justifyContent\\":\\"center\\"}} -->
<div class=\\"wp-block-buttons\\"><!-- wp:button /--></div>
<!-- /wp:buttons -->"
`;

exports[`Buttons block justify content sets Justify items left option 1`] = `
"<!-- wp:buttons {\\"layout\\":{\\"type\\":\\"flex\\",\\"justifyContent\\":\\"left\\"}} -->
<div class=\\"wp-block-buttons\\"><!-- wp:button /--></div>
<!-- /wp:buttons -->"
`;

exports[`Buttons block justify content sets Justify items right option 1`] = `
"<!-- wp:buttons {\\"layout\\":{\\"type\\":\\"flex\\",\\"justifyContent\\":\\"right\\"}} -->
<div class=\\"wp-block-buttons\\"><!-- wp:button /--></div>
<!-- /wp:buttons -->"
`;

exports[`Buttons block when a button is shown adjusts the border radius 1`] = `
"<!-- wp:buttons -->
<div class=\\"wp-block-buttons\\"><!-- wp:button {\\"style\\":{\\"border\\":{\\"radius\\":\\"6px\\"}}} -->
<div class=\\"wp-block-button\\"><a class=\\"wp-block-button__link\\" href=\\"\\" style=\\"border-radius:6px\\">Hello</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->"
`;
120 changes: 75 additions & 45 deletions packages/block-library/src/buttons/test/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,59 +27,89 @@ afterAll( () => {
} );
} );

describe( 'when a button is shown', () => {
it( 'adjusts the border radius', async () => {
const initialHtml = `<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button {"style":{"border":{"radius":"5px"}}} -->
<div class="wp-block-button"><a class="wp-block-button__link" style="border-radius:5px" >Hello</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->`;
const { getByA11yLabel } = await initializeEditor( {
initialHtml,
} );
describe( 'Buttons block', () => {
describe( 'when a button is shown', () => {
it( 'adjusts the border radius', async () => {
const initialHtml = `<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button {"style":{"border":{"radius":"5px"}}} -->
<div class="wp-block-button"><a class="wp-block-button__link" style="border-radius:5px" >Hello</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->`;
const { getByA11yLabel } = await initializeEditor( {
initialHtml,
} );

const buttonsBlock = await waitFor( () =>
getByA11yLabel( /Buttons Block\. Row 1/ )
);
fireEvent.press( buttonsBlock );
const buttonsBlock = await waitFor( () =>
getByA11yLabel( /Buttons Block\. Row 1/ )
);
fireEvent.press( buttonsBlock );

// onLayout event has to be explicitly dispatched in BlockList component,
// otherwise the inner blocks are not rendered.
const innerBlockListWrapper = await waitFor( () =>
within( buttonsBlock ).getByTestId( 'block-list-wrapper' )
);
fireEvent( innerBlockListWrapper, 'layout', {
nativeEvent: {
layout: {
width: 100,
// onLayout event has to be explicitly dispatched in BlockList component,
// otherwise the inner blocks are not rendered.
const innerBlockListWrapper = await waitFor( () =>
within( buttonsBlock ).getByTestId( 'block-list-wrapper' )
);
fireEvent( innerBlockListWrapper, 'layout', {
nativeEvent: {
layout: {
width: 100,
},
},
},
} );

const buttonInnerBlock = await waitFor( () =>
within( buttonsBlock ).getByA11yLabel( /Button Block\. Row 1/ )
);
fireEvent.press( buttonInnerBlock );

const settingsButton = await waitFor( () =>
getByA11yLabel( 'Open Settings' )
);
fireEvent.press( settingsButton );

const radiusStepper = await waitFor( () =>
getByA11yLabel( /Border Radius/ )
);

const incrementButton = await waitFor( () =>
within( radiusStepper ).getByTestId( 'Increment' )
);
fireEvent( incrementButton, 'onPressIn' );

expect( getEditorHtml() ).toMatchSnapshot();
} );
} );

const buttonInnerBlock = await waitFor( () =>
within( buttonsBlock ).getByA11yLabel( /Button Block\. Row 1/ )
);
fireEvent.press( buttonInnerBlock );
describe( 'justify content', () => {
[
'Justify items left',
'Justify items center',
'Justify items right',
].forEach( ( justificationOption ) =>
it( `sets ${ justificationOption } option`, async () => {
const initialHtml = `<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button /--></div>
<!-- /wp:buttons -->`;
const { getByA11yLabel, getByText } = await initializeEditor( {
initialHtml,
} );

const settingsButton = await waitFor( () =>
getByA11yLabel( 'Open Settings' )
);
fireEvent.press( settingsButton );
const block = await waitFor( () =>
getByA11yLabel( /Buttons Block\. Row 1/ )
);
fireEvent.press( block );

const radiusStepper = await waitFor( () =>
getByA11yLabel( /Border Radius/ )
);
fireEvent.press(
getByA11yLabel( 'Change items justification' )
);

// Select alignment option
fireEvent.press(
await waitFor( () => getByText( justificationOption ) )
);

const incrementButton = await waitFor( () =>
within( radiusStepper ).getByTestId( 'Increment' )
expect( getEditorHtml() ).toMatchSnapshot();
} )
);
fireEvent( incrementButton, 'onPressIn' );

const expectedHtml = `<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button {"style":{"border":{"radius":"6px"}}} -->
<div class="wp-block-button"><a class="wp-block-button__link" href="" style="border-radius:6px">Hello</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->`;
expect( getEditorHtml() ).toBe( expectedHtml );
} );
} );
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ For each user feature we should also add a importance categorization label to i
-->

## Unreleased
- [**] Fix content justification attribute in Buttons block [#37887]

## 1.69.0
- [*] Give multi-line block names central alignment in inserter [#37185]
Expand Down