Skip to content

Conversation

@antonis
Copy link
Contributor

@antonis antonis commented May 4, 2022

Fixes #16408

Description

This PR enlarges the the thumbnails of the site design picker by dynamically setting the layout dimension. This is achieved by:

  1. defining a common dimension provider interface for both layout/design pickers
  2. passing a different implementation for the Page Layout picker
    and the Site Design picker
  3. setting the desired preview thumbnail dimensions programmatically

The PR also sets the loading skeleton dimensions to match those of the thumbnails on each screen.

Note:
Other alternative approaches were considered/tried. One involved creating separate layout files but due to the structure of the code this required duplicating the implementation of the LayoutCategoryAdapter and its nested components.

To test

Start the site creation flow and verify that the thumbnails are enlarged

Before (feature/site-design-revamp) After
Screenshot_20220504_120733 Screenshot_20220504_190123

Verify that the UI of the page layout picker is unchanged

Before (19.8) After
2Screenshot_20220504_120843 Screenshot_20220504_120827

Regression Notes

  1. Potential unintended areas of impact
    Page Layout Picker UI

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing and the existing ModalLayoutPickerViewModelTest

  3. What automated tests I added (or what prevented me from doing so)
    Relied on existing tests

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@antonis antonis added [Type] Task Site Creation Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. labels May 4, 2022
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 4, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@antonis antonis changed the title Task/16408 enlarge thumbnails [Site Design Revamp] Enlarges design thumbnails May 4, 2022
@antonis antonis marked this pull request as ready for review May 4, 2022 09:03
@antonis antonis requested review from a team and mkevins May 4, 2022 09:03
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 4, 2022

You can test the changes on this Pull Request by downloading the APKs:

@twstokes
Copy link
Contributor

twstokes commented May 4, 2022

👋 I've updated the issue to include the exact dimensions from the designs, which it looks like you're matching for mobile (200px). 👍

I'm not sure if Android will need to handle tablet widths, which appear to be 250px.

@antonis
Copy link
Contributor Author

antonis commented May 4, 2022

👋 I've updated the issue to include the exact dimensions from the designs, which it looks like you're matching for mobile (200px). 👍

I'm not sure if Android will need to handle tablet widths, which appear to be 250px.

Thank you for pointing this out @twstokes 🙇
I've added a different dimension for tablet with 3f080de

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.

I tested this on a Pixel 3a (physical device) for both the page and site design pickers, in both portrait and landscape modes, and it is working as described. Also, the code changes look great.

Thank you for also mentioning the alternative approaches, and the rationale for going with this approach. I completely agree with the rationale, and appreciate the effort to find the cleanest way forward, reducing the potential for tensions to arise between the usages of these shared implementations. Great work Antonis!

One minor observation (not a blocker, but perhaps just something to confirm), I noticed that in the screenshots for the page picker provided in the description:

screenshots

Verify that the UI of the page layout picker is unchanged

Before (19.8) After
2Screenshot_20220504_120843 Screenshot_20220504_120827

the first category (Blog) seems slightly smaller in the "after" image (i.e. the horizontal rules seem closer together vertically compared to the "before" image). I didn't notice this in my testing, but only when looking closely at the provided screenshots, so perhaps this is more of an issue with the screenshots themselves? I also notice the clock is offset a bit, so I'm thinking it's a screenshot issue, and not an implementation issue.

@antonis
Copy link
Contributor Author

antonis commented May 5, 2022

Thank you for reviewing @mkevins 🙇

the first category (Blog) seems slightly smaller in the "after" image (i.e. the horizontal rules seem closer together vertically compared to the "before" image)
I hadn't noticed that but I think it has to do with the way the markdown table is rendered since when I open the full size images the match on the size of the rows.
Screenshot 2022-05-05 at 12 38 16 PM
I noticed thought the issue with the subtitle width/margin (notice that the 2nd line breaks on a different place) 🤔
I'll proceed with merging this PR and look into it separately since this seem to exist on our feature branch already.

@antonis antonis merged commit 159b716 into feature/site-design-revamp May 5, 2022
@antonis antonis deleted the task/16408-enlarge-thumbnails branch May 5, 2022 09:44
@mkevins
Copy link
Contributor

mkevins commented May 5, 2022

when I open the full size images the match on the size of the rows

Thanks for confirming 👍 I figured it must be from the way it was displayed, since the clock was not aligned either, and that's not part of "our" UI 😄 .

@antonis antonis mentioned this pull request May 24, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. Site Creation [Type] Task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants