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

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

Merged
merged 15 commits into from
Mar 25, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a17a5c6
Refactor heading block to share more code with web version.
SergioEstevao Mar 9, 2020
b4e84f9
Merge branch 'master' into rnmobile/add_align_to_heading_block
SergioEstevao Mar 11, 2020
28350f5
Remove branching for RN.
SergioEstevao Mar 11, 2020
d8fb1e8
Remove platform checks at block level.
SergioEstevao Mar 11, 2020
bebc055
Fix lint-errors.
SergioEstevao Mar 11, 2020
2f21f8b
Implement Dropdown menu for React Native.
SergioEstevao Mar 21, 2020
733f4f6
Increase headers icon size.
SergioEstevao Mar 21, 2020
d6aedfa
Show all heading levels on the block controls in RN
SergioEstevao Mar 21, 2020
3ec775c
Merge branch 'master' into rnmobile/add_align_to_heading_block
SergioEstevao Mar 21, 2020
2d57fe2
Style separators to have left margin.
SergioEstevao Mar 24, 2020
0988634
Color selected checkmark on cell
SergioEstevao Mar 25, 2020
5050d3d
Add separators for dropdown menus
SergioEstevao Mar 25, 2020
3d7e007
Remove extra padding on panel for dropdown
SergioEstevao Mar 25, 2020
507f972
Merge branch 'master' into rnmobile/add_align_to_heading_block
SergioEstevao Mar 25, 2020
bc3ca73
Fix separator on iOS and Android.
SergioEstevao Mar 25, 2020
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
View file
Open in desktop
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,
};
}
13 changes: 9 additions & 4 deletions packages/block-library/src/heading/edit.js
View file
Open in desktop
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,
Expand Down Expand Up @@ -54,17 +54,19 @@ function HeadingEdit( {
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
}
isCollapsed={ Platform.OS === 'web' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do mobile toolbars support isCollapsed prop? Wht happens if we keep it true for mobile too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't show., it looks collapsed is equivalent to hidden in mobile.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should update the toolbar implementation to just ignore that prop instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can update the code for the native ToolbarGroupCollapsed to simple do the same as the non-collapsed version, until we get a proper implementation of a collapsed toolbar for mobile.

/>
<AlignmentToolbar
value={ align }
onChange={ ( nextAlign ) => {
setAttributes( { align: nextAlign } );
} }
isCollapsed={ Platform.OS === 'web' }
/>
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Heading settings' ) }>
<p>{ __( 'Level' ) }</p>
<Text variant="label">{ __( 'Level' ) }</Text>
<HeadingToolbar
isCollapsed={ false }
minLevel={ 1 }
Expand All @@ -81,7 +83,9 @@ function HeadingEdit( {
<RichText
ref={ ref }
identifier="content"
tagName={ Block[ tagName ] }
tagName={
Platform.OS === 'web' ? Block[ tagName ] : tagName
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Block component should have an equivalent in native. This is going to become a central component to write blocks. Ideally all new blocks use it. cc @ellatrix

Copy link
Member

Choose a reason for hiding this comment

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

Yes, ideally native has a Block component too. I'm not sure what it will take there. I don't know enough about the native side, but happy to collaborate with someone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working on to have an equivalent ( more a mock at this stage) on this PR: #20772

Will update this PR when the above is merged in.

}
value={ content }
onChange={ ( value ) =>
setAttributes( { content: value } )
Expand All @@ -103,6 +107,7 @@ function HeadingEdit( {
[ `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
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions packages/components/src/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,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
6 changes: 6 additions & 0 deletions packages/components/src/text/emotion-css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* External dependencies
*/
import css from '@emotion/css';

export default css;
6 changes: 6 additions & 0 deletions packages/components/src/text/emotion-css.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* External dependencies
*/
import { css } from '@emotion/native';

export default css;
5 changes: 1 addition & 4 deletions packages/components/src/text/mixins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
/**
* External dependencies
*/
import css from '@emotion/css';
/**
* Internal dependencies
*/
import { fontFamily } from './font-family';
import css from './emotion-css';
SergioEstevao marked this conversation as resolved.
Show resolved Hide resolved

const fontWeightNormal = `font-weight: 400;`;
const fontWeightSemibold = `font-weight: 600;`;
Expand Down