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

Make "blockquote" more cross platform #15769

Conversation

hypest
Copy link
Contributor

@hypest hypest commented May 22, 2019

Description

Extra PR on top of #15482 to make the Quote block cross-platform. Introduces a BlockQuote component that is implemented differently on web and native mobile.

How has this been tested?

Tested via #15482.

Also, tested locally with ./bin/setup-local-env.sh and npm run dev and the web version of the blockquote seems to work fine.

Types of changes

Introduce a BlockQuote (local) component. On the native mobile implementation of it it's a Fragment that returns augmented versions of the 2 children expected. Each children is augmented to add the tagName prop needed for properly rendering the two RichTexts on gutenberg-mobile.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@hypest hypest 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 22, 2019
@hypest hypest added this to the 5.8 (Gutenberg) milestone May 22, 2019
@hypest hypest requested a review from SergioEstevao May 22, 2019 07:51
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.

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

@@ -0,0 +1,8 @@
const BlockQuote = ( props ) => (
Copy link
Member

Choose a reason for hiding this comment

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

Can you follow what @koke did in #14361?

export const HorizontalRule = 'hr';

@hypest
Copy link
Contributor Author

hypest commented May 22, 2019

Thank you for having a look @gziolo, will follow your suggestions!

@hypest hypest changed the title Rnmobile/blockquote port to mobile cross platform Make "blockquote" more cross platform May 22, 2019
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.

We talked a bit about this on Slack, but I'll expand here. I don't think this makes sense in its current state as a component. The web version might, as a platform-agnostic way to wrap things around a blockquote element, but the native variant makes too many assumptions about their children: it has 0 or 1, RichText elements, the first is a blockquote, the second a citation.

Besides this breaking if the childrens ever change, it seems the only reason it's different is to inject the blockquote/cite tagName into those RichText fields. But the reason that's not done in edit.js is because those are not the elements used. If you inspect a quote on the web, you'd see this (simplified, there are many more nested divs):

<blockquote>
    <div>
        <p>Hello world</p>
        <p>Another paragraph</p>
    </div>
    <div>
        <p>Me</p>
    </div>
</blockquote>

However, native tries to replicate this as:

<Fragment>
    <blockquote>
        <p>Hello world</p>
        <p>Another paragraph</p>
    </blockquote>
    <cite>
        <p>Me</p>
    </cite>
</Fragment>

I imagine the goal is to reuse the existing Aztec styles for quotes, but the styling is so different that we'll probably want to change it anyway

Web:
Screen Shot 2019-05-22 at 11 35 42

iOS:

IMG_FDD3C279DDED-1

I think we'll need to pass the desired font styles to Aztec instead of relying on a specific HTML tag to render what we want.

@hypest
Copy link
Contributor Author

hypest commented May 22, 2019

Closing in favor of #15482 since there are now additional changes there to make the quote block cross platform.

@hypest hypest closed this May 22, 2019
@youknowriad youknowriad deleted the rnmobile/blockquote_port_to_mobile-cross-platform branch May 22, 2019 14:06
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants