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

[Mobile] - Full-width/wide alignment support for more blocks #25184

Merged

Conversation

geriux
Copy link
Member

@geriux geriux commented Sep 9, 2020

Gutenberg Mobile PR -> wordpress-mobile/gutenberg-mobile#2605
WordPress iOS PR -> wordpress-mobile/WordPress-iOS#14865
WordPress Android PR -> wordpress-mobile/WordPress-Android#12916

Description

This PR is a continuation of #24598 it adds full-width/wide alignment for all blocks that support them except the Columns block which is being added in another PR.

Blocks that support full-width/wide alignments: Video, Latest-posts, Gallery, Media & text, Columns, Pullquote, Cover, Group, and Image. (Columns will be added soon as well)

How has this been tested?

Steps to test ---

With metro running:

Video

  • With a premium or self-hosted site
  • Add a Video block
  • Change the alignment of the block to Full width
  • Expect to see the block edge-to-edge of the screen
  • Change the alignment to wide
  • Expect to see some margin between the block and the edges of the screen

Latest-posts

  • Add a Latest-posts block
  • Change the alignment of the block to Full width
  • Expect to see the block edge-to-edge of the screen
  • Change the alignment to wide
  • Expect to see some margin between the block and the edges of the screen

Gallery

  • Add a Gallery block, select some images
  • Change the alignment of the block to Full width
  • Expect to see the block edge-to-edge of the screen
  • Change the alignment to wide
  • Expect to see some margin between the block and the edges of the screen

Gallery within a Group block

  • Add a Group block and change its alignment to Full width
  • Within the Group block, a Gallery block, select some images
  • Change the alignment of the block to Full width
  • Expect to see the block edge-to-edge of the screen
  • Change the alignment to wide
  • Expect to see some margin between the block and the edges of the screen

Screenshots

Video, Latest-posts
iOS Android
iPad iOS Safe area
Gallery, Pullquote
iOS Android
iPad iOS Safe area
Media & Text
iOS Android
iPad iOS Safe area

Types of changes

New feature

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@geriux geriux added [Status] In Progress Tracking issues with work in progress Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Sep 9, 2020
@geriux geriux changed the title [Mobile] - Full-width/wide alignment options support for more blocks [Mobile] - Full-width/wide alignment support for more blocks Sep 9, 2020
@github-actions
Copy link

github-actions bot commented Sep 9, 2020

Size Change: -2 B (0%)

Total Size: 1.18 MB

Filename Size Change
build/edit-widgets/index.js 21.2 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 668 B 0 B
build/block-directory/index.js 8.55 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 129 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 8.65 kB 0 B
build/block-library/editor.css 8.65 kB 0 B
build/block-library/index.js 135 kB 0 B
build/block-library/style-rtl.css 7.66 kB 0 B
build/block-library/style.css 7.65 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.5 kB 0 B
build/components/index.js 169 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.42 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 685 B 0 B
build/data/index.js 8.6 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.7 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.29 kB 0 B
build/edit-post/style.css 6.27 kB 0 B
build/edit-site/index.js 20.4 kB 0 B
build/edit-site/style-rtl.css 3.84 kB 0 B
build/edit-site/style.css 3.84 kB 0 B
build/edit-widgets/style-rtl.css 3 kB 0 B
build/edit-widgets/style.css 3 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.6 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@geriux
Copy link
Member Author

geriux commented Sep 9, 2020

Hey @iamthomasbishop 👋

I'm working on adding full-width/wide support for more blocks, I think overall all of them look ok but I had to add some margins to Gallery block when it's in full-width to avoid some issues with the appender, I saw that the web editor does this as well but I'd like to know what you think about it or if you have any other ideas for it.

This is how the Gallery block it's looking in full-width (you could also test the build I made here)

iPhone

Portrait Landscape

iPad

Portrait Landscape

Web

What do you think? Would you expect the images to reach the actual border of the screen or having some margins as they have in the screenshots?

Thanks!

@iamthomasbishop
Copy link

@geriux Tested out the build and it feels really solid overall. I was going to point out the Gallery issue, but you already mentioned it 😄

to avoid some issues with the appender

I think overall, I expected the gallery image cells to be flush against the edges, so if there's a way around it then great. But if there are good reasons for adding some margin then that's fine -- what types of issues are we talking about? 😄 Is this because the image cells have padding inside them (which would force us to use negative margins on the left/right)?

One (small) thing that I noted was that I expected the Text side of the Media & Text block to be flush to the edges on portrait phone, same as the Media side (example), but I'm not sure if that's a real issue or just personal preference.

And this isn't necessarily an expectation because it's not a thing on the web, but I wish the Separator block had these width controls (allowing you to force full-width separator, for example). Is that something we can force? or would it require getting something build on Core first?

@geriux
Copy link
Member Author

geriux commented Sep 11, 2020

Hey, @iamthomasbishop thank you so much for testing and your feedback!

I think overall, I expected the gallery image cells to be flush against the edges, so if there's a way around it then great. But if there are good reasons for adding some margin then that's fine -- what types of issues are we talking about? 😄 Is this because the image cells have padding inside them (which would force us to use negative margins on the left/right)?

So the appender for Gallery block is within the MediaPlaceholder component, the first approach I took was to add margins to it depending on the width of the appender and the screen width, if they'd match I added some margins. That was working until I remembered about the Safe Area and that complicated things plus wide alignment since this component doesn't "know" if the block using it has any alignments set. But I can definitely give it another look now knowing that we'd prefer to reach the edges in full-width.

Regarding to full-width, if we make it reach the edges, how should it look like in landscape with safe area?

By removing the margins, it overlaps the "selected blue border". Would this be an issue?

One (small) thing that I noted was that I expected the Text side of the Media & Text block to be flush to the edges on portrait phone, same as the Media side (example), but I'm not sure if that's a real issue or just personal preference.

We could make all InnerBlocks from Media & Text reach the edges if it has full-width selected, were you think of something like this?:

I didn't remove them completely because then the Paragraph block's content would be cut off.

And this isn't necessarily an expectation because it's not a thing on the web, but I wish the Separator block had these width controls (allowing you to force full-width separator, for example). Is that something we can force? or would it require getting something build on Core first?

Well, I did a quick search to see if there were plans for this and there's an open PR already 🎉 Although looks like there's still some conversations about it here.

If that PR gets merged the alignment options would be available for mobile as well since they share the same component.

@iamthomasbishop
Copy link

@geriux thanks for the context on the challenges wrt. I was just testing the gallery w/ a few themes and I see what you mean with the forced margins on left/right. I'm on the fence about what to do, because while the current state of this PR achieves parity with the web editor (not-quite full-width), the front-end does extend fully to the edges — so it's a matter of deciding, do we align with the web editor or the front-end output?

At this point, for me it comes down to whether or not it'll add a lot of work. If yes, I think we can leave it as-is and align with the web (assuming the result on the front-end is edge-to-edge like web). But if it isn't a lot of work then I think it'd be nice to go edge-to-edge in order to align with the front-end result.

Regarding to full-width, if we make it reach the edges, how should it look like in landscape with safe area?

I think I'd expect it to look basically look the same as you showed in your landscape screenshot above, but with the border to be "above" the image in terms of z-index. I think we have that same bordering conflict on full-width image blocks, so perhaps we can resolve that separately. Here's an example of the image block with caption focused, where the border is inset but the z-index of the image itself covers the border (I'd like the border to be on top of the image, in these cases).

We could make all InnerBlocks from Media & Text reach the edges if it has full-width selected, were you think of something like this?:

Ahh, on second thought we might want to leave the other blocks as-is. I think we may see some other side-effects so let's not make it any more complex than we have to right now. 😄

Well, I did a quick search to see if there were plans for this and there's an open PR already 🎉 Although looks like there's still some conversations about it here.

If that PR gets merged the alignment options would be available for mobile as well since they share the same component.

Awesome, let's just wait and see, then! 🤞

@geriux
Copy link
Member Author

geriux commented Sep 23, 2020

Hey @iamthomasbishop thanks for the feedback!

At this point, for me it comes down to whether or not it'll add a lot of work. If yes, I think we can leave it as-is and align with the web (assuming the result on the front-end is edge-to-edge like web). But if it isn't a lot of work then I think it'd be nice to go edge-to-edge in order to align with the front-end result.

I was able to make it work reaching the edges and solving the issue I had with the appender 🎉. So no margins needed =)

I think I'd expect it to look basically look the same as you showed in your landscape screenshot above, but with the border to be "above" the image in terms of z-index. I think we have that same bordering conflict on full-width image blocks, so perhaps we can resolve that separately. Here's an example of the image block with caption focused, where the border is inset but the z-index of the image itself covers the border (I'd like the border to be on top of the image, in these cases).

I changed the border to be on top of everything fixing the issue I encountered in landscape (with a safe area) and also the one you mentioned with the Image block.

Ahh, on second thought we might want to leave the other blocks as-is. I think we may see some other side-effects so let's not make it any more complex than we have to right now. 😄

Good, no changes then!

I made a new build for you to test whenever you have a chance. Thanks!

@iamthomasbishop
Copy link

@geriux Gave the latest build on iOS a quick test (will test on Android as well) and it's feeling really great.

I was able to make it work reaching the edges and solving the issue I had with the appender 🎉. So no margins needed =)

Awesome 😄

I changed the border to be on top of everything fixing the issue I encountered in landscape (with a safe area) and also the one you mentioned with the Image block.

Doubly awesome! // cc @lukewalczak Can we replicate the border behavior here, where on full-width, the border is basically inset (not cut off) and on the highest z-index?

Beyond that, can we cross-check with @lukewalczak to make sure our approaches between this work and Luke's work on Columns are aligned? I'm guessing we're sharing some of the same logic between the two, but let's make sure we're not letting something fall through the cracks 😄

@geriux
Copy link
Member Author

geriux commented Sep 25, 2020

Beyond that, can we cross-check with @lukewalczak to make sure our approaches between this work and Luke's work on Columns are aligned? I'm guessing we're sharing some of the same logic between the two, but let's make sure we're not letting something fall through the cracks 😄

Sure! I'll check his PR.

Thanks for the review!

@geriux geriux marked this pull request as ready for review September 25, 2020 12:19
@geriux geriux removed the [Status] In Progress Tracking issues with work in progress label Sep 25, 2020
Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

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

Tested on both platforms and works correctly, as expected 🎉 Left small nit! Good job!

@geriux
Copy link
Member Author

geriux commented Oct 5, 2020

Tested on both platforms and works correctly, as expected 🎉 Left small nit! Good job!

Thanks for reviewing and testing Luke!

Hey @iamthomasbishop 👋 is there anything else you want to review before merging this? Let me know, Thanks!

@geriux geriux merged commit 20aa0af into master Oct 8, 2020
@geriux geriux deleted the rnmobile/feature/full-width-wide-align-support-more-blocks branch October 8, 2020 16:10
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