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

Port blockquote block to RN mobile #15482

Merged
merged 18 commits into from
May 24, 2019

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented May 7, 2019

Description

Implements the quote block in react native.

This is done with minimal changes on the quote block edit code.
It implements a new Blockquote component that in the web continues to wrap the richtext using a blockquote element. On the native side it uses a View to implement the necessary styling and wrapping of the RichText component.

It also changes the RichText in native to a have a default tagName of div this allows the RichText component using inside the quote block to not to be changed and allow in native the proper wrapping of internal elements and final removal of the root tag.

This PR implement a specific native version of the edit of the block because of the following:

- Use of an HTML element, <blockquote> to set the style of the richtext
- Use of two richtext elements but not define the top element of each of them, so this need to be changed for mobile in order for their style to be presented correctly.

This can be tested using the following PR on GB-mobile:wordpress-mobile/gutenberg-mobile#963

@SergioEstevao SergioEstevao requested a review from hypest May 21, 2019 15:36
@SergioEstevao SergioEstevao added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label May 21, 2019
@SergioEstevao SergioEstevao added this to the 5.8 (Gutenberg) milestone May 21, 2019
@SergioEstevao SergioEstevao marked this pull request as ready for review May 21, 2019 15:39
@hypest
Copy link
Contributor

hypest commented May 22, 2019

This seems to work nicely @SergioEstevao , nice one!

By the way, I think we might have an opportunity here to make this block cross-platform. I've opened #15769 with some early experiments towards that. Have a look and we can chat about this!


export const Blockquote = ( props ) => {
return (
<View style={ { flex: 1, flexDirection: 'row', alignItems: 'stretch' } } >
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 this can be simpler, Views in react native seem to support border styles:

<View style={ styles.wpBlockQuote } >
	{ props.children }
</View>
.wpBlockQuote {
	border-left-width: 4px;
	border-left-color: $black;
	border-left-style: solid;
	padding-left: 8px;
	margin-left: 8px;
}

I'll let Thomas review the exact colors, but the web version seems to be using black. If you're seeing a blue line, that's coming from Twenty Nineteen

@@ -809,6 +809,7 @@ 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 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

@@ -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?

Copy link
Contributor

@koke koke left a comment

Choose a reason for hiding this comment

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

This looks pretty good for a first PR. Since the writing flow isn't ready to ship yet, make sure we only enable this for Debug builds, like the other in progress blocks

/**
* 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>

@SergioEstevao SergioEstevao requested a review from mkaz as a code owner May 24, 2019 09:30
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comment. Refactoring for web part looks great. It should make porting Pullqote block much easier as it also uses blockquote HTML tag.

packages/components/CHANGELOG.md Outdated Show resolved Hide resolved
@gziolo gziolo added the [Package] Components /packages/components label May 24, 2019
Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>
@SergioEstevao
Copy link
Contributor Author

@koke do you want to give it another look, or can it be merged it in?

@@ -3,6 +3,7 @@
*/
import { __ } from '@wordpress/i18n';
import { AlignmentToolbar, BlockControls, RichText } from '@wordpress/block-editor';
import { BlockQuotation } from '@wordpress/components';
Copy link
Contributor

@youknowriad youknowriad May 24, 2019

Choose a reason for hiding this comment

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

I wonder if we should gather all these Platform specific components in a separate package or otherwise under the same parent variable.

Whether it's import { Blockquote, Div } from '@wordpress/platform'

or

import { PlatformComponents } from '@wordpress/components';
const { Blockquote, Div } = PlatformComponents;

Copy link
Member

@gziolo gziolo May 24, 2019

Choose a reason for hiding this comment

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

Yep, this is a good point. I think we should extract all those components into their own package and maybe re-export from @wordpress/components for backward compatibility only. It's definitely out of the scope of this PR @SergioEstevao. Anyway, it's something we should decide on quite soon as you will need to use more HTML tags like:

  • figure, img
  • ul, ol, li
  • div

@gziolo
Copy link
Member

gziolo commented May 24, 2019

I see Travis detected some issue with style import:

Error: File to import not found or unreadable: ./quote/style.scss.

Needs to be fixed to unblock merge.

@SergioEstevao
Copy link
Contributor Author

I move the style file to be in the BlockQuotation package. It looks something was using it too.

@SergioEstevao SergioEstevao merged commit 95f51b4 into master May 24, 2019
@SergioEstevao SergioEstevao deleted the rnmobile/blockquote_port_to_mobile branch May 24, 2019 11:08
@@ -851,6 +859,11 @@ const RichTextContainer = compose( [
selectionStart.clientId === clientId &&
selectionStart.attributeKey === identifier
);
} else {
isSelected = isSelected && (
Copy link
Contributor

Choose a reason for hiding this comment

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

This change (the addition of the "else" block with its setting of the isSelected flag) seems to be causing wordpress-mobile/gutenberg-mobile#1031. @SergioEstevao , do you recall what this change was added to do? I'm going to open a PR to remove it but, I'm afraid I might be missing some context and the removal might add some other problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SergioEstevao added a comment about this here: #15833 (comment)

@@ -0,0 +1,19 @@
.wp-block-quote {
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 style for the block, I don't think it should have been moved to the component.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are correct. I missed it...

Copy link
Member

Choose a reason for hiding this comment

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

This file was never referenced so it only needs to be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

#15845 fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants