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 media-text respect stacking setting on native #17682

Merged
merged 5 commits into from
Oct 10, 2019

Conversation

koke
Copy link
Contributor

@koke koke commented Oct 1, 2019

Description

After #14364, the Media & Text block is stacked by default on mobile. The native implementation was wrongly assuming that mobile == native and stacking even on larger screens. Besides, it would still set the width for each column to 50%, so it would look wrong.

gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1400

How has this been tested?

Tested on WordPress for iOS and web.

  • Media should appear above text at full width on small screens.
  • Media should appear next to text (as usual) in larger screens.

Screenshots

Native:

media-text-stack

Web:

media-text-stacking-web

Types of changes

Beyond the fix specific to Media & Text, this completes the native implementation of @wordpress/viewport.

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.

@koke koke requested review from gziolo and mkevins and removed request for gziolo October 2, 2019 15:22
@koke koke added [Block] Media & Text Affects the Media & Text Block [Package] Viewport /packages/viewport Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Oct 2, 2019
@koke koke marked this pull request as ready for review October 2, 2019 15:23
@koke
Copy link
Contributor Author

koke commented Oct 4, 2019

Testing on WPiOS:

  1. Insert a media-text block
  2. Tap 'Add image or video'
  3. Choose any of the options (device or media library)
  4. Cancel the picker without selecting an image
  5. Get a red screen:
undefined is not an object (evaluating 'media.id')

<unknown>
    index.native.js:88:39
invokeCallbackAndReturnFlushedQueue
    [native code]:0

@mkevins
Copy link
Contributor

mkevins commented Oct 8, 2019

I tested this on a Pixel 3a with Android 10 (portrait and landscape), as well as in a local web install (normal mode, and emulating mobile, portrait + landscape via Chrome Devtools), and everything works as described. 🎉

Screenshots:

Portrait Landscape
Image on left Portrait-left Landscape-left
Image on right Portrait-right Landscape-right

The issue described here: #17682 (comment) seems to have been resolved here: ace01c9

Very nice work @koke , and thanks @geriux for adding the null-check. 👍

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

It's very nice seeing the dimension event listener working well on mobile in this PR! 😃

Nice work @koke 🎉

@koke koke merged commit d229012 into master Oct 10, 2019
@koke koke deleted the rnmobile/media-text-stacked branch October 10, 2019 07:53
@youknowriad youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Viewport /packages/viewport
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants