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

Remove feature flag for mobile page templates #20718

Merged
merged 7 commits into from
Mar 13, 2020
Merged

Conversation

koke
Copy link
Contributor

@koke koke commented Mar 9, 2020

We are ready to launch Starter Page Templates on mobile (#18055). Since we ended up with a mobile-only implementation for now, I'm removing the feature flag

Fixes wordpress-mobile/gutenberg-mobile#1962

gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1987
gutenberg PR: #20718
WPiOS PR: wordpress-mobile/WordPress-iOS#13595
WPAndroid PR: wordpress-mobile/WordPress-Android#11409

cc @iamthomasbishop

@koke koke 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 Mar 9, 2020
@github-actions
Copy link

github-actions bot commented Mar 9, 2020

Size Change: -14 B (0%)

Total Size: 856 kB

Filename Size Change
build/editor/index.js 43.8 kB -14 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 100 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/index.js 111 kB 0 B
build/block-library/style-rtl.css 7.38 kB 0 B
build/block-library/style.css 7.39 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.21 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.64 kB 0 B
build/edit-post/style.css 8.64 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.45 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 779 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@koke koke requested a review from pinarol March 9, 2020 11:25
@koke
Copy link
Contributor Author

koke commented Mar 9, 2020

✅ The envelope emoji is not looking right on Android:

![Screenshot_20200309-125151](https://user-images.githubusercontent.com/8739/76210818-2eef1380-6205-11ea-82f0-36be0eae6def.png)

Fixed in 3272b04

@koke
Copy link
Contributor Author

koke commented Mar 9, 2020

On Android (Moto G5 Plus), if I focus on the paragraph, the border overlaps the template picker:

Screenshot_20200309-133235

@pinarol
Copy link
Contributor

pinarol commented Mar 9, 2020

On Android (Moto G5 Plus), if I focus on the paragraph, the border overlaps the template picker:

I thought this was already discussed when we dropped the first design which had the buttons under the paragraph block. Because this is pretty much an expected result of sticking the buttons to the keyboard. When the viewport is small things will overlap, same in the landscape mode:

IMG_3619

BTW I think in reality it is more clear that the picker buttons are moving with the keyboard, so the overlapping didn't feel that buggy to me, also, I was able to close the keyboard to avoid overlapping

However, could we use a similar approach to "BlockInsertionPoint" to show the picker buttons? So we'd completely avoid overlapping? @koke

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

LGTM! Tested with WPiOS, WPAndroid.

  • Apply/cancel template WPiOS
  • Apply/cancel template WPAndroid
  • Dark mode WPiOS
  • Checked template contents&emojis on WPiOS
  • Checked template contents&emojis on WPAndroid

Let's also get a 👍 from @iamthomasbishop

@koke
Copy link
Contributor Author

koke commented Mar 9, 2020

When the viewport is small things will overlap

That makes sense, I think what I found surprising was the border above the buttons. I would expect the border to have scrolled behind the picker, or even the block to have a white background that partially covered the picker. This way it looks like something is wrong in the Z axis

@pinarol
Copy link
Contributor

pinarol commented Mar 9, 2020

Or, it can be a result of transparency. On iOS I observe that the border is below the buttons:

IMG_3620

On Android I am not able to get to a similar state because the screen is too small in Landscape mode, and too big in Portrait.

@iamthomasbishop
Copy link

@koke along with the landscape feedback @pinarol mentioned above, here is some feedback from my initial spin through the test build:

UI

  • Can we add ~12px of padding at the top of the page content? I believe we are applying additional padding on top of the post/page title in the editor, so this would align with that (example: https://cloudup.com/cbMvzmdHb6a)
  • The App Bar seems a bit too tall compared to the native one (~9 or 10px too tall). Can we shorten it up slightly to align more with the native one? I believe 56px tall should work across platforms (example: https://cloudup.com/c-hVuBGgeVv)
  • [iOS] Dark Mode: border-bottom on the navigation bar is a bit too strong. We can probably dim it down a bit?
  • [iOS] Dark Mode: I wonder if we should apply a dark gray bg on the Navigation Bar to match the native one. Thoughts @kokejb?
  • Can we display a Snackbar/Notice briefly after the template is applied? Something like this: https://cloudup.com/cDYvDtvc38y
  • Can we apply a 300ms fadeOut to the inline picker buttons when they disappear?
  • When title already exists, applying the template shouldn’t override the existing title

Blocks

  • Lack of alignment controls on Button block feels weird (example: https://cloudup.com/cIR3RJqFDNm)
  • Can we remove the dashed borders from Spacer blocks on the Template Preview? They feel unnecessary during preview
  • Should images on image blocks be full-width? (Example: https://cloudup.com/c3c0uNa5Ncx)
  • On Services layout: This is my fault because I prepped the contents, but the Spacer below Focus section should be 24px, not 32 (example: https://cloudup.com/c7pv9iOZjR4)

@pinarol
Copy link
Contributor

pinarol commented Mar 10, 2020

Lack of alignment controls on Button block feels weird (example: https://cloudup.com/cIR3RJqFDNm)

I am a bit confused about this one because we had decided this way: wordpress-mobile/gutenberg-mobile#1790 (comment) This doesn’t necessarily mean we should change that plan, right? @iamthomasbishop

@koke
Copy link
Contributor Author

koke commented Mar 10, 2020

here is some feedback from my initial spin through the test build

Thanks for the detailed feedback @iamthomasbishop. Could you specify which ones would you consider blockers? There are some of those that seem like quick fixes, but others would be more challenging and probably handled better as a second iteration on it. Particularly:

Can we display a Snackbar/Notice briefly after the template is applied?

It seems doable, but I don't recall having a similar thing anywhere in gutenberg-mobile, so we'd have to build the component, or bridge the existing native one.

Can we apply a 300ms fadeOut to the inline picker buttons when they disappear?

I haven't done any animations so far. My understanding is that this would be relatively easy, but we might be in for a surprise 😅

Lack of alignment controls on Button block feels weird

I imagine we'd just have to update the templates to use buttons when #20191 is merged

Can we remove the dashed borders from Spacer blocks on the Template Preview?

Removing them on preview only might complicate things. Could we remove them in all cases?

Should images on image blocks be full-width?

I'm seeing them full width on my iPhone. Do you get that behaviour consistently? 🤔

@iamthomasbishop
Copy link

Could you specify which ones would you consider blockers?

@koke Honestly, I wouldn't consider any of them "complete" blockers. And while I obviously would prefer to solve as many as possible prior to shipping, here are the few that I would like to be solved prior to shipping if were to ship today (in order of priority):


Your Questions

It seems doable, but I don't recall having a similar thing anywhere in gutenberg-mobile, so we'd have to build the component, or bridge the existing native one.

Ok, fair enough. I was hoping we'd be able to bridge to the existing component, but if not let's not worry about it.

I haven't done any animations so far. My understanding is that this would be relatively easy, but we might be in for a surprise 😅

Sounds good. Honestly, it's not anything critical, but it may be a nice little improvement if it doesn't require a bunch of work 😄

I imagine we'd just have to update the templates to use buttons when #20191 is merged

That's right, should have mentioned that in my previous comment (cc @pinarol, as you were asking here).

Removing them on preview only might complicate things. Could we remove them in all cases?

I think we could remove the dashed borders for the static state of the Spacer block altogether, but I don't feel super strongly about it. I was thinking the dashed border would be useful to provide a clear tap target and an indication as to what is taking up that space, rather than just have whitespace, but now it kinda feels unnecessary and repetitive in most cases. With that said, it's not specific to this feature, so maybe we can consider it separately.

I'm seeing them full width on my iPhone. Do you get that behaviour consistently? 🤔

I should have been a little more specific — I'm seeing this only on the Team template, and testing primarily on an iPhone 11 Pro. This is what I'm seeing (in the template preview and in the editor once applied). FWIW, on iPad (full-screen and split-view) the images span the full viewport width. 🤷‍♂


Other questions

  • Do we expect the Cover block to be merged in time for us shipping this? If so, we might also want to consider utilizing it in the templates (if we feel compelled to do so)

@pinarol
Copy link
Contributor

pinarol commented Mar 12, 2020

Do we expect the Cover block to be merged in time for us shipping this? If so, we might also want to consider utilizing it in the templates (if we feel compelled to do so)

@iamthomasbishop We added gradient and video background support as well! 🎉 But we'll need a final design review from you for removing the DEV flag, so I am not certain about when it can be publicly available yet.

@koke
Copy link
Contributor Author

koke commented Mar 12, 2020

I'm seeing this only on the Team template, and testing primarily on an iPhone 11 Pro. This is what I'm seeing (in the template preview and in the editor once applied).

That's what I understood, but I'm seeing something different. I tried with the iPhone 11 Pro simulator in case it was device specific, but I'm still seeing them full width. https://cloudup.com/c0j0S2N5xvs

I'm not sure what's going on but maybe there was some glitch calculating the image sizes and now it's cached? Can you try uninstalling the build and installing again and see if it's still happens.

I'm not sure what else to try, since I can't make it happen 😞

@iamthomasbishop
Copy link

That's what I understood, but I'm seeing something different. I tried with the iPhone 11 Pro simulator in case it was device specific, but I'm still seeing them full width. https://cloudup.com/c0j0S2N5xvs

Very interesting 🤔 I've been bouncing between various alpha builds, so I don't think re-installing will solve anything — but I'll give it a try!

@koke
Copy link
Contributor Author

koke commented Mar 13, 2020

I'm going to go ahead with this PR and move all the items to separate issues.

On Services layout: This is my fault because I prepped the contents, but the Spacer below Focus section should be 24px, not 32

Fixed here in ab3ea1a

  • Can we add ~12px of padding at the top of the page content? I believe we are applying additional padding on top of the post/page title in the editor, so this would align with that (example: https://cloudup.com/cbMvzmdHb6a)
  • The App Bar seems a bit too tall compared to the native one (~9 or 10px too tall). Can we shorten it up slightly to align more with the native one? I believe 56px tall should work across platforms (example: https://cloudup.com/c-hVuBGgeVv)
  • [iOS] Dark Mode: border-bottom on the navigation bar is a bit too strong. We can probably dim it down a bit?
  • [iOS] Dark Mode: I wonder if we should apply a dark gray bg on the Navigation Bar to match the native one. Thoughts @kokejb?
  • Can we apply a 300ms fadeOut to the inline picker buttons when they disappear?

wordpress-mobile/gutenberg-mobile#2012

  • When title already exists, applying the template shouldn’t override the existing title

Moved to wordpress-mobile/gutenberg-mobile#2013

I understand we're not doing this for now?

It should resolve once buttons block ships, although it'll require updating the templates

  • Can we remove the dashed borders from Spacer blocks on the Template Preview? They feel unnecessary during preview

@iamthomasbishop I'm not sure if we made a decision here

Still investigating, I can't reproduce the issue

@koke koke merged commit f8421c4 into master Mar 13, 2020
@koke koke deleted the mobile-enable-page-templates branch March 13, 2020 10:32
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 13, 2020
@iamthomasbishop
Copy link

I understand we're not doing this for now?

(re: snackbar) That's correct.

I'm not sure if we made a decision here

(re: removing dashed borders from static Spacer blocks)

I think we can handle that separately, as an iteration on the Spacer block itself.

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.

Make SPTs(Layouts) public
3 participants