WordPress-iOS icon indicating copy to clipboard operation
WordPress-iOS copied to clipboard

Fix oversize label in no result side menu on iPad

Open ipalm0423 opened this issue 3 years ago • 4 comments
trafficstars

Fixes #19244 #19246

Description:

This PR fixes the issue where the labels are oversizing in its container view.

Before After
iPad Q1 Q1-fix
iPad-RTL Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-09-19 at 01 17 04 Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-09-19 at 01 12 12

Root Cause:

In the xib NoResults.storyboard, the container view of the labels only has the width constraint Width <= 360, but it doesn't have a constraint related to its super view. This will let the labels go outside if its super view is smaller than the label's content. Constraint on superView In this case, the side menu view only has width == 320. The container view will still go up to 360 because it doesn't care about its super view size.

Fix:

  • Add missing width constraint between the container view and its super view

Test Step:

  1. Launch the app.
  2. Log in with an account without any notification or sign up for a new account.
  3. Navigate to the Notifications or My Site tab.
  4. Notice the text should fit inside the left layout.

Regression Notes

  1. Potential unintended areas of impact
  • I list all impacted pages in here with screenshots.
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • Manually test on all impacted pages on iPad, and key pages on iPhone, and list the test result here
  1. What automated tests I added (or what prevented me from doing so) add unitTest at 41c4cb3900b9b4893dcabdbace0ca2c7a3d3246d for testing labels width in different sizes of the container.

PR submission checklist:

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

ipalm0423 avatar Sep 13 '22 19:09 ipalm0423

Any views that present NoResultViewController

👋 @ipalm0423 - do you think we should list out the views that use NoResultsViewController so when potentially non-technical testers test these changes, they'll know some to try? Thanks!

twstokes avatar Sep 17 '22 00:09 twstokes

Any views that present NoResultViewController

👋 @ipalm0423 - do you think we should list out the views that use NoResultsViewController so when potentially non-technical testers test these changes, they'll know some to try? Thanks!

Sure, I created a view list that is inspected on this PR.

Also, I added some tests in the 41c4cb3900b9b4893dcabdbace0ca2c7a3d3246d. To test the NoResultsViewController label width in different width of container.

ipalm0423 avatar Sep 18 '22 15:09 ipalm0423

CI branch: https://github.com/wordpress-mobile/WordPress-iOS/pull/19333

twstokes avatar Sep 21 '22 18:09 twstokes

This looks really good @ipalm0423! Thanks for being so thorough with your manual tests and screenshots. 🙇 The unit tests are great, too.

I noticed the margins on this view were a little tight. Do you think we should tweak this?

Yes, we could tweak the horizontal margin to make it look nice. Do we have the UI design specification for this margin size? (Figma, Zeplin...etc?)

I only found this spec, but not sure if it's the correct one.

ipalm0423 avatar Sep 24 '22 04:09 ipalm0423

Yes, we could tweak the horizontal margin to make it look nice. Do we have the UI design specification for this margin size? (Figma, Zeplin...etc?)

I only found this spec, but not sure if it's the correct one.

Typically we would pull in a designer for Figma designs, but how do you feel about adding just a few points of margin on the sides so it's not as snug?

twstokes avatar Sep 26 '22 19:09 twstokes

Typically we would pull in a designer for Figma designs, but how do you feel about adding just a few points of margin on the sides so it's not as snug?

I tweaked the margin for 20 on f05670ec1022b74c5209a3a61933966ddcacd2df , also modify the unit-test

Tested on the narrow key pages:

margin 20 default RTL
iPhone iphone13- Simulator Screen Shot - iPhone 13 - 2022-09-28 at 16 52 54
iPad iPad9 ipad9-rtl

ipalm0423 avatar Sep 28 '22 08:09 ipalm0423

I tweaked the margin for 20 on f05670e , also modify the unit-test

Great! Thanks for doing that. I've pushed the latest to the CI branch to run tests. 👍

twstokes avatar Sep 29 '22 02:09 twstokes

Warnings
:warning: This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by :no_entry_sign: dangerJS

@ipalm0423 can we update RELEASE-NOTES.txt for these changes? Thanks!

No, problem. Rebased to trunk, and update the release note at ec96ae642b249bd35cf81b1b8dce3182b8151c3d.

ipalm0423 avatar Sep 29 '22 09:09 ipalm0423

Latest has been pushed to CI branch.

twstokes avatar Sep 30 '22 00:09 twstokes