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

Update existing templates to use new blocks #2179

Merged
merged 12 commits into from
May 11, 2020

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Apr 24, 2020

Fixes #2168

Related PR

gutenberg: WordPress/gutenberg#21857

Description

This PR updates the mobile pages templates to use new blocks. It also adds some additional starter content.

To test:

  1. Start a new page, then repeat the following steps for these templates:
    • About
    • Blog
    • Services
  2. Tap page template in picker (on the bottom of the screen)
  3. Apply the template
  4. Scroll through to ensure the content is using the blocks described here
  5. Ensure additional content listed here has been added

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mkevins
Copy link
Contributor Author

mkevins commented May 1, 2020

Hi @iamthomasbishop 👋 😄

Thanks for providing the additional content described here. I've added that content, and updated the templates with all the new blocks.

Here's how the About template looks in desktop preview after it is applied

spt-about-desktop

Note: the button block alignment is a known issue.

I noticed that we are using slightly different images for the second and third images within the services template (focus and strategy) compared to your mockup. I also noticed a vertical alignment issue (which is probably related, since the image heights differ). Lastly, I see a word wrap issue when we use all 4 columns, so maybe we can reduce that down? Wdyt?

Services template applied and previewed in desktop mode

spt-services-desktop

Can you provide the images for strategy and focus, if we are to switch them too?

@mkevins mkevins added the [Status] Needs Design Review Needs design review or sign-off before shipping label May 1, 2020
@mkevins mkevins marked this pull request as ready for review May 1, 2020 07:36
@iamthomasbishop
Copy link
Contributor

Thanks for the update @mkevins ! I had completely forgotten that on mobile we are only showing a max of 2 columns per row when in landscape orientation even though this results in 4 pretty-narrow columns on desktop. Perhaps we shouldn’t add a fourth column, after all. I think it does look a tad awkward on landscape phone and tablet, but perhaps we can adjust the Columns block to allow up to 3 columns per row (not as part of this work, but as a separate iteration on the Columns block). I think this would probably solve most of the concerns you had in your latest comment.

cc @pinarol @lukewalczak @dratwas — I know we had a lengthy discussion while working on the Columns/Column block about how many column blocks per row we allow, but I think 2 is proving to be a bit too constricting. Should we consider increasing the maximum to 3 columns per row?

I noticed that we are using slightly different images for the second and third images within the services template (focus and strategy) compared to your mockup

I didn’t swap in updated images for the columns in my mockups, only chose new images for each of the new columns I added. So what already exists in code is what we should continue using. 👍

@pinarol
Copy link
Contributor

pinarol commented May 5, 2020

cc @pinarol @lukewalczak @dratwas — I know we had a lengthy discussion while working on the Columns/Column block about how many column blocks per row we allow, but I think 2 is proving to be a bit too constricting. Should we consider increasing the maximum to 3 columns per row?

I don't have a strong preference actually, but if you think that would be better let's do that as part of this issue. I expect it'd be a straight forward change. We can also consider making the readable content area bigger than 580 in some wide screen devices and fit there even more columns.

@mkevins
Copy link
Contributor Author

mkevins commented May 5, 2020

Hi @iamthomasbishop 👋 😃

I've reverted the addtion of the 4th column in the services template, so it is back to having 3 columns.

Services template with 3 columns - desktop preview

spt-services-desktop2

I've left the 4 columns with the quotes, since they didn't seem to have wrapping issues. Please let me know if you wanted that reverted as well, or if it's ok to leave it as is. In case of the latter, should we still add the Needs Copy Review label to the original issue for the quote?

@pinarol
Copy link
Contributor

pinarol commented May 6, 2020

Can we accelerate the review of this one please? @iamthomasbishop @chipsnyder Let's target merging this till EOD Fri (May 8th) to make it to the next release.

Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

From a code perspective, this looks good 👍.

For the landscape mode on the Services template, the third column wraps to a row by itself and "Inspiration" and "Strategy" don't seem to be aligned. This looks like it's related to the size of the photo though so doesn't appear to be new.

Tested on iOS 13 iPhone 11

Simulator Screen Shot - iPhone 11 - 2020-05-06 at 11 38 35

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented May 6, 2020

Services template with 3 columns - desktop preview

@mkevins Ah, shoot. Just realized that first image is taller than the others — is there a way we can crop that image at the bottom to be the same height/width as the other images?

I've left the 4 columns with the quotes, since they didn't seem to have wrapping issues.

I would also move this back to 3 columns. It doesn’t look terrible, but wider columns might make for more readable text.

@mkevins
Copy link
Contributor Author

mkevins commented May 7, 2020

Hi @iamthomasbishop 👋

I'll remove that 4th quote then. Thanks for the guidance.

is there a way we can crop that image at the bottom to be the same height/width as the other images?

We can use the width parameter with Pexels to limit the width of the image, but it seems the problem is coming from their different aspect ratios (the first image has a ratio of 4:3, and the other two have ratios ~ 3:2). If you can provide a similar Pexels image for the first one, with a 3:2 aspect, I can replace it.

@iamthomasbishop
Copy link
Contributor

@mkevins Here are some updated images, all cropped to 3:2 so we can ensure consistent sizing.

@mkevins
Copy link
Contributor Author

mkevins commented May 8, 2020

@mkevins Here are some updated images, all cropped to 3:2 so we can ensure consistent sizing.

Thanks Thomas! I've updated the images and removed the 4th column in the about template, and everything is lining up nicely now 🎉 .

Template Landscape Desktop Preview
About spt-about-landscape spt-about-desktop2
Services spt-services-landscape spt-services-desktop3

Copy link
Contributor

@iamthomasbishop iamthomasbishop left a comment

Choose a reason for hiding this comment

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

@mkevins Everything is looking good to me based on the latest screenshots — nice work!

@pinarol pinarol merged commit 50dcc5b into develop May 11, 2020
@pinarol pinarol deleted the issue/2168-update-existing-templates-for-new-blocks branch May 11, 2020 17:40
@pinarol
Copy link
Contributor

pinarol commented May 11, 2020

Thank you everyone! I went ahead and merged it when CI was green.

@mchowning mchowning mentioned this pull request May 11, 2020
5 tasks
@mkevins
Copy link
Contributor Author

mkevins commented May 11, 2020

Thanks for testing and reviewing, and thanks Pinar for wrangling the merge! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Page Templates [Status] Needs Design Review Needs design review or sign-off before shipping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update existing templates using the new blocks
4 participants