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

Port blockquote block to RN mobile #15482

Merged
merged 18 commits into from
May 24, 2019
Merged

Port blockquote block to RN mobile #15482

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
6f02cba
Enable quote block in editor.
SergioEstevao Apr 30, 2019
8dc7c06
Add mobile version of quote edit.
SergioEstevao Apr 30, 2019
116cf9b
Merge branch 'master' into rnmobile/blockquote_port_to_mobile
SergioEstevao May 15, 2019
ae65dfc
Stop reading focus from context.
SergioEstevao May 15, 2019
3bc05ac
Make default tag to be div like on the web.
SergioEstevao May 22, 2019
6507230
Implement block quote component in web and RN.
SergioEstevao May 22, 2019
4d1de79
Simplify native block quote component.
SergioEstevao May 22, 2019
f5ebb76
Implement empty Alignment toolbar.
SergioEstevao May 22, 2019
536442e
Merge branch 'master' into rnmobile/blockquote_port_to_mobile
SergioEstevao May 22, 2019
587a01b
Merge branch 'master' into rnmobile/blockquote_port_to_mobile
SergioEstevao May 23, 2019
03be5ad
Fix style import.
SergioEstevao May 23, 2019
f28f013
Remove import
SergioEstevao May 23, 2019
a1a16f4
Rename blockquote to BlockQuotation.
SergioEstevao May 24, 2019
08b2327
Move BlockQuotation to be a primate component.
SergioEstevao May 24, 2019
d8b77ba
Add documentation reference for BlockQuotation.
SergioEstevao May 24, 2019
1f8a12a
Add BlockQuotation info to the changeling.
SergioEstevao May 24, 2019
1f2e231
Update packages/components/CHANGELOG.md
SergioEstevao May 24, 2019
889116f
Restore style file.
SergioEstevao May 24, 2019
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/components/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export { default as BlockEdit } from './block-edit';
export { default as BlockFormatControls } from './block-format-controls';
export * from './colors';
export * from './font-sizes';
export { default as AlignmentToolbar } from './alignment-toolbar';
export { default as InspectorControls } from './inspector-controls';
export { default as PlainText } from './plain-text';
export {
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/rich-text/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,14 +809,14 @@ export class RichText extends Component {
RichText.defaultProps = {
formattingControls: [ 'bold', 'italic', 'link', 'strikethrough' ],
format: 'string',
tagName: 'div',
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 replicating the behaviour of the web where there the default tagName is a Div.

This allows the blockquote block to work properly because the

are surrounded by the div.

Our other blocks should work correctly because they define a tagName and don't use the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

are surrounded by the div

Hmm, how do the divs play a role in the native mobile case, considering that this change is in the .native.js file? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the file we expect a tagName in several places to be defined in order to be able to produce the HTML. It also allows the wrapping of the multiple

that can exist in a blockquote inside a root tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, so it's for proper support of the save method of the block?

Let's add this expanded explanation to the PR description @SergioEstevao , cool?

};

const RichTextContainer = compose( [
withInstanceId,
withBlockEditContext( ( { clientId, onFocus, isSelected, onCaretVerticalPositionChange }, ownProps ) => {
withBlockEditContext( ( { clientId, onFocus, onCaretVerticalPositionChange }, ownProps ) => {
return {
clientId,
isSelected,
onFocus: onFocus || ownProps.onFocus,
onCaretVerticalPositionChange,
};
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export const registerCoreBlocks = () => {
nextpage,
separator,
list,
quote,
].forEach( ( { metadata, name, settings } ) => {
registerBlockType( name, {
...metadata,
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/quote/blockquote.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const Blockquote = 'blockquote';
16 changes: 16 additions & 0 deletions packages/block-library/src/quote/blockquote.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* External dependencies
*/
import { View } from 'react-native';
/**
* Internal dependencies
*/
import styles from './style';

export const Blockquote = ( props ) => {
return (
<View style={ styles.wpBlockQuote } >
{ props.children }
</View>
);
};
8 changes: 6 additions & 2 deletions packages/block-library/src/quote/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
*/
import { __ } from '@wordpress/i18n';
import { AlignmentToolbar, BlockControls, RichText } from '@wordpress/block-editor';
/**
* Internal dependencies
*/
import { Blockquote } from './blockquote';
Copy link
Contributor

Choose a reason for hiding this comment

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

Following @gziolo's comment on the other PR, I'm not 100% sure about this naming.

I don't think this should be a standalone component, but having Blockquote inside of the quote block is a bit confusing to me. I think I'd suggest something along the lines of "Container" or "Wrapper" but I'd like to hear from @gziolo as well.

Copy link
Member

Choose a reason for hiding this comment

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

I shared my thoughts already:

Ideally, this new component would be renamed to BlockQuotation and placed inside @wordpress/components package (packages/components/primitives/block-quotation.js?) together with the corresponding documentation. See the prior art from @koke for HorizontalRule implemented in #14361.

Related MDN page which I used to reason about the name of the component: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/blockquote

Copy link
Member

Choose a reason for hiding this comment

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

You will have to run into the same issue when porting Pullquote block. It is also something that plugin developers will need as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that, while this gets translated into a blockquote HTML element in the web version, I don't see it being a good standalone "quote" component right now, like the HorizontalRule is.

The current web implementation only looks like a quote because it's (IMO, wrongly) stealing the styles for the quote block, which won't work when we want to do pullquote as well, and the native version is defining a non-configurable style as well.

Also, those styles are only applied to the blockquote because of the wp-block-quote class, but there's no specific mention of blockquote in the CSS. From what I see, all blockquote provides on the web is semantic markup, but nothing more over a generic div. Which is why, more abstractly, I wonder if it would make sense to make this a generic View/Container element, where one can pass a tagName prop for semantic reasons on the web implementation.

We still have the unresolved issue that we have to use inline styles for native, and CSS classes for the web, but otherwise the code could be pretty similar for both:

<View tagName="blockquote" className={ className } style={ style }>
    <RichText
        identifier="value"
        // ...
    />
    { ( ! RichText.isEmpty( citation ) || isSelected ) && (
        <RichText
	    identifier="citation"
            // ...
	/>
    ) }
</View>


export default function QuoteEdit( { attributes, setAttributes, isSelected, mergeBlocks, onReplace, className } ) {
const { align, value, citation } = attributes;
Expand All @@ -16,7 +20,7 @@ export default function QuoteEdit( { attributes, setAttributes, isSelected, merg
} }
/>
</BlockControls>
<blockquote className={ className } style={ { textAlign: align } }>
<Blockquote className={ className } style={ { textAlign: align } }>
<RichText
identifier="value"
multiline
Expand Down Expand Up @@ -54,7 +58,7 @@ export default function QuoteEdit( { attributes, setAttributes, isSelected, merg
className="wp-block-quote__citation"
/>
) }
</blockquote>
</Blockquote>
</>
);
}
7 changes: 7 additions & 0 deletions packages/block-library/src/quote/style.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.wpBlockQuote {
border-left-width: 4px;
border-left-color: $black;
border-left-style: solid;
padding-left: 8px;
margin-left: 8px;
}
1 change: 1 addition & 0 deletions packages/viewport/src/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as withViewportMatch } from './with-viewport-match';
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 think we need this one anymore right?

44 changes: 44 additions & 0 deletions packages/viewport/src/with-viewport-match.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* External dependencies
*/
import { mapValues } from 'lodash';

/**
* WordPress dependencies
*/
import { createHigherOrderComponent } from '@wordpress/compose';
import { withSelect } from '@wordpress/data';

/**
* Higher-order component creator, creating a new component which renders with
* the given prop names, where the value passed to the underlying component is
* the result of the query assigned as the object's value.
*
* @see isViewportMatch
*
* @param {Object} queries Object of prop name to viewport query.
*
* @example
*
* ```jsx
* function MyComponent( { isMobile } ) {
* return (
* <div>Currently: { isMobile ? 'Mobile' : 'Not Mobile' }</div>
* );
* }
*
* MyComponent = withViewportMatch( { isMobile: '< small' } )( MyComponent );
* ```
*
* @return {Function} Higher-order component.
*/
const withViewportMatch = ( queries ) => createHigherOrderComponent(
withSelect( ( ) => {
return mapValues( queries, ( query ) => {
return query === 'mobile';
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned on Slack, this isn't behaving like the web version. For now, let's make the AlignmentToolbar a fake component on native, and leave that and the viewport implementation for wordpress-mobile/gutenberg-mobile#198

} );
} ),
'withViewportMatch'
);

export default withViewportMatch;