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

Site Design Revamp - Add Caption to Recommended Section #18710

Merged

Conversation

Gio2018
Copy link
Contributor

@Gio2018 Gio2018 commented May 23, 2022

Fixes #18674

This PR adds a caption to the recommended section in the Site Design Picker

To test:

  • checkout/build/run this branch
  • Start the site creation flow and get to the design picker (either skip the vertical selection or select any vertical)
  • Make sure that the recommended section (the one with larger thumbnails on top) has a "PICKED FOR YOU" caption
  • Make sure the other sections do not have captions
  • Cancel the site creation flow, tap the FAB then "Site Page"
  • Make sure the page layout picker does not have captions in any section

Regression Notes

  1. Potential unintended areas of impact
    Low - Page design picker uses part of the same code

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    test that the page design picker was not impacted

  3. What automated tests I added (or what prevented me from doing so)
    none
    PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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.

@Gio2018 Gio2018 added this to the 20.0 milestone May 23, 2022
@Gio2018 Gio2018 requested review from twstokes and a team May 23, 2022 22:32
@Gio2018 Gio2018 self-assigned this May 23, 2022
@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 23, 2022

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18710-760d80f on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 23, 2022

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18710-760d80f on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

This looks good @Gio2018! 🚀 I tested different accessibility sizes as well as VoiceOver.

In the Figma designs by @kyleaparker it looks like the font size is defined as 13px with a 590 weight. Using .subheadline puts the font at 15pt(px) and I also see we're defining .semibold for the weight.

.caption1 is defined as 12px in the HIG with regular weight. .caption1 with .semibold is visually closest to me, but I wanted to get your thoughts.

Designs

It's best to view these screenshots fullscreen to get a sense of scale.

Subheadline Caption 1 Regular Caption 1 Semibold

@Gio2018
Copy link
Contributor Author

Gio2018 commented May 24, 2022

Thanks @twstokes !

.caption1 is defined as 12px in the HIG with regular weight. .caption1 with .semibold is visually closest to me, but I wanted to get your thoughts.

I thought about that, but it looks a bit unreadable. Thoughts @kyleaparker ?

@Gio2018 Gio2018 requested a review from twstokes May 24, 2022 17:26
Giorgio Ruscigno added 2 commits May 24, 2022 17:09
…read the caption from the section property
…_:cellForRowAt:), remove unnecessary call to set the cell caption
@Gio2018 Gio2018 merged commit 68cad84 into feature/site-design-revamp May 25, 2022
@Gio2018 Gio2018 deleted the task/18674-recommended-designs-caption branch May 25, 2022 00:25
@kyleaparker
Copy link

.caption1 is defined as 12px in the HIG with regular weight. .caption1 with .semibold is visually closest to me, but I wanted to get your thoughts.

I think subheadline is probably the best here but we may need to add some margin underneath

@twstokes
Copy link
Contributor

I think subheadline is probably the best here but we may need to add some margin underneath

Sounds good! We have a UI tweaks issue that I'll add this note to. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants