Skip to content

UI enhancements for jetpack focus fullscreen overlay in landscape mode #17510

Merged
AjeshRPai merged 9 commits intotrunkfrom
issue/17333-jetpack-focus-fullscreen-overlay-landscape-mode
Nov 23, 2022
Merged

UI enhancements for jetpack focus fullscreen overlay in landscape mode #17510
AjeshRPai merged 9 commits intotrunkfrom
issue/17333-jetpack-focus-fullscreen-overlay-landscape-mode

Conversation

@AjeshRPai
Copy link
Copy Markdown
Contributor

@AjeshRPai AjeshRPai commented Nov 22, 2022 •
edited
Loading

Part of #17333

This PR changes the following

  • Adds the landscape XML for Jetpack full-screen overlay, makes the content scrollable
  • updates the Portrait XML to handle large fonts for accessibility, the content is made scrollable
  • fixes Jetpack fullscreen overlay being shown on top of the already shown jetpack fullscreen overlay on rotation

To test:

Pre- Requisites
Launch WP app -> Go to my site -> App settings -> Debug settings -> Enable jp_removal_one flag

Check if the fonts are displayed properly on Properly in Landscape and Portrait

  1. Launch the app -> Go to my site -> Access stats/reader/notification feature
  2. Verify that the UI is shown properly
Landscape Portrait
Screenshot_20221122_173149 Screenshot_20221122_173214

Check if the fonts are displayed properly on increasing the size of the fonts

  1. Go to system settings -> Display and font size -> Increase both display size and font size
  2. Launch the app -> Go to my site -> Access stats/reader/notification feature
  3. Verify that the UI is shown properly
Landscape(Scrollable) Portrait
Screenshot_20221122_173436 Screenshot_20221122_173422

Check if the multiple full screen overlay is displayed when rotated

  • Launch the app -> Go to my site -> Access stats/reader/notification feature
  • Rotate the device when overlay is shown
  • Verify that on dismissing the overlay, no other overlay is displayed below it.

⚠️ Merge Instructions

  • Revert the commit added for testing purposes 6d092ae
  • Remove not read for merge label
  • Merge as normal

Regression Notes

  1. Potential unintended areas of impact

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

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

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.

@AjeshRPai AjeshRPai added this to the 21.3 milestone Nov 22, 2022
@AjeshRPai AjeshRPai self-assigned this Nov 22, 2022
@AjeshRPai AjeshRPai changed the base branch from trunk to issue/17330-jetpack-focus-create-generic-fullscreen-overlay November 22, 2022 11:13
@AjeshRPai AjeshRPai mentioned this pull request Nov 22, 2022
Closed
3 tasks
@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Nov 22, 2022 •
edited
Loading

WordPress📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17510-56d4fb8.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit56d4fb8
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Nov 22, 2022 •
edited
Loading

Jetpack📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17510-56d4fb8.apk
💡 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit56d4fb8
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@AjeshRPai AjeshRPai added the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Nov 22, 2022
@AjeshRPai AjeshRPai marked this pull request as ready for review November 22, 2022 12:52
@osullivanchris
Copy link
Copy Markdown

osullivanchris commented Nov 22, 2022

Good work @AjeshRPai it looks pretty good considering the limitations. Here's some suggested tweaks/enhancements:

  • Maybe the heading can have a max size. It already starts with quite large, legible type. So it might not need to scale equally with the subheading.
  • On landscape mode could we remove the top padding above the logo/icons, so that we see at least some text before the fold? Here's how it looks to me. It would be better if the default scroll position could look like your screenshot here where I assume you have scrolled down a bit.
    image
  • If I make the text and display large, and change the language to German, the button copy is getting cut off. Do we need a dynamic/set max button text size? Or could we change the copy in languages when it gets too long?
    image

@AjeshRPai
Copy link
Copy Markdown
Contributor Author

AjeshRPai commented Nov 22, 2022 •
edited
Loading

@osullivanchris Thanks chris for the review.

I have made the following changes as per your review comments.

Maybe the heading can have a max size. It already starts with quite large, legible type. So it might not need to scale equally with the subheading.

  1. I have changed the scaling of the heading to have a maximum size of 32 sp. I haven't changed the scaling of the caption text

Screenshot of the device with max font size and display size 👇🏼

Screenshot_20221122_193453

On landscape mode could we remove the top padding above the logo/icons, so that we see at least some text before the fold? Here's how it looks to me. It would be better if the default scroll position could look like your screenshot here where I assume you have scrolled down a bit.

Yep, I have decreased the top margin of the heading to 16dp from 24dp. Let me know if this looks better. Also I have adjusted the padding between the logo and the heading below it from 24 dp to 16dp.

Screenshot of the device in landscape mode with padding adjusted 👇🏼

Screenshot_20221122_193842

If I make the text and display large, and change the language to German, the button copy is getting cut off. Do we need a dynamic/set max button text size? Or could we change the copy in languages when it gets too long?

I tested by setting the button size to a cap of 14 sp(default size is 12 sp I believe), and increasing the display size and font size and setting the language to german. The text still doesn't fit the button on Portrait mode. I don't think we can handle the transition + large font size on the button. I think the only solution is to change the copy in languages when it gets too long. Right now the max font size on the button is set to 14sp.

Screenshot of the device in portrait mode with button text size set to 14 sp and display and font size set to maximum

Screenshot_20221122_193453

I have pushed the above changes with the commit - b0ff534

@AjeshRPai
Copy link
Copy Markdown
Contributor Author

AjeshRPai commented Nov 22, 2022 •
edited
Loading

hey @zwarm @JB184351 👋🏼

Since @osullivanchris is doing the design review. Testing the following scenario by one of you should be enough to merge this PR. Please feel free to ignore this PR if there is 2 approvals.

Check if the multiple full screen overlay is displayed when rotated

  • Launch the app -> Go to my site -> Access stats/reader/notification feature
  • Rotate the device when overlay is shown
  • Verify that on dismissing the overlay, no other overlay is displayed below it.

@JB184351
Copy link
Copy Markdown

JB184351 commented Nov 22, 2022

hey @zwarm @JB184351 👋🏼

Since @osullivanchris is doing the design review. Testing the following scenario by one of you should be enough to merge this PR. Please feel free to ignore this PR if there is 2 approvals.

Check if the multiple full screen overlay is displayed when rotated

  • Launch the app -> Go to my site -> Access stats/reader/notification feature
  • Rotate the device when overlay is shown
  • Verify that on dismissing the overlay, no other overlay is displayed below it.

Verified each of these with no issues, looks good to me!

Base automatically changed from issue/17330-jetpack-focus-create-generic-fullscreen-overlay to trunk November 22, 2022 18:17
This was referenced Nov 22, 2022
Merged
Merged
@osullivanchris
Copy link
Copy Markdown

osullivanchris commented Nov 23, 2022

@AjeshRPai Hey nice work, thanks for those improvements. Its looking better.

On landscape on larger display sizes we're still not seeing much information. We could

  • reduce the max font size even more specifically for landscape
  • hide the logos when the display size is large, on landscape
    Do you think it is worth further work?

Re; longer strings in other languages. It would be better in that case if we got a shorter string. I assume it would work if we said "Get Jetpack", "Get App". But given that is not our ideal copy, how would you propose we get that shorter title for e.g. German? Would we ask for a custom translation for German? Or could we have two strings, and get translations for both. The first string would be preferable (Get the Jetpack App), the second string would be a backup string (Get app) if the copy is too long in another language. Let me know if you have a process/method in mind here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Nov 23, 2022

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@AjeshRPai
Merge branch 'trunk' into issue/17333-jetpack-focus-fullscreen-overla…
56d4fb8 
…y-landscape-mode

# Conflicts:
#	WordPress/src/main/res/values/dimens.xml
@AjeshRPai
Copy link
Copy Markdown
Contributor Author

AjeshRPai commented Nov 23, 2022

@osullivanchris 👋🏼

I have

  • disabled the scaling of heading in landscape mode
  • decreased the padding between heading and caption in landscape mode

Commit - bbb2d61

I think hiding the logo is not worth it. Since the content is scrollable and most of the users won't be using max display size + font size in landscape mode, I think it's okay to ignore that corner case.

@AjeshRPai
Copy link
Copy Markdown
Contributor Author

AjeshRPai commented Nov 23, 2022 •
edited
Loading

Hey @osullivanchris

I have created this Issue for tracking the big fonts on the button.

I will take it up later as a separate PR. I am merging this PR for now.

@AjeshRPai AjeshRPai merged commit c80265f into trunk Nov 23, 2022
@AjeshRPai AjeshRPai deleted the issue/17333-jetpack-focus-fullscreen-overlay-landscape-mode branch November 23, 2022 13:49
@ParaskP7 ParaskP7 mentioned this pull request May 18, 2023
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zwarm zwarm Awaiting requested review from zwarm

2 more reviewers

@JB184351 JB184351 JB184351 approved these changes

@osullivanchris osullivanchris osullivanchris approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AjeshRPai AjeshRPai

Labels

Jetpack [Status] Needs Design Review A designer needs to sign off on the implemented design.

Projects

None yet

Milestone

21.3

Development

Successfully merging this pull request may close these issues.

4 participants

@AjeshRPai @wpmobilebot @osullivanchris @JB184351