Skip to content

Make media-text respect stacking setting on native#17682

Merged
koke merged 5 commits intomasterfrom
rnmobile/media-text-stacked
Oct 10, 2019
Merged

Make media-text respect stacking setting on native#17682
koke merged 5 commits intomasterfrom
rnmobile/media-text-stacked

Conversation

@koke
Copy link
Copy Markdown
Contributor

@koke koke commented Oct 1, 2019 •
edited
Loading

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 koke mentioned this pull request Oct 2, 2019
Merged
1 task
@koke
Copy link
Copy Markdown
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

@koke koke mentioned this pull request Oct 4, 2019
Merged
5 tasks
@mkevins
Copy link
Copy Markdown
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. 👍

mkevins
mkevins approved these changes Oct 8, 2019
View reviewed changes
Copy link
Copy Markdown
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
@Tug Tug mentioned this pull request Dec 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkevins mkevins mkevins approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

No one assigned

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

Milestone

Gutenberg 6.7

Development

Successfully merging this pull request may close these issues.

3 participants

@koke @mkevins @youknowriad